HMPID condition paramters calibration using DCS DPs by gvolpe79 · Pull Request #9171 · AliceO2Group/AliceO2

gvolpe79 · 2022-06-13T21:37:32Z

No description provided.

shahor02 · 2022-06-21T22:34:26Z

+      startValidity = std::chrono::duration_cast<std::chrono::milliseconds>(timeNow.time_since_epoch()).count(); // in ms
+    }
+
+    mProcessor->setStartValidity(startValidity);


you are setting startvalidity each time you receive something, i.e. at least every 5 s (when the full DPs image is sent even if nothing was changed). Should not you use as a start validity the beginning of the integration period?

shahor02 · 2022-06-21T22:41:04Z

+// Called At the end of the Stream of DPs, in
+// HMPIDDCSDataProcessorSpec::endOfStream() function
+void HMPIDDCSProcessor::finalize()
+{


As far as I can see, you create a CCDB object only in the endOfStream, but this means never, as the workflow is running permanently, w/o any correlation with data taking (SOR/EOR).
In case you need to perform some action at the EOR, you can use https://github.com/AliceO2Group/AliceO2/blob/dev/Detectors/DCS/include/DetectorsDCS/RunStatusChecker.h, see how EMCAL does this:

AliceO2/Detectors/EMCAL/workflow/include/EMCALWorkflow/EMCALDCSDataProcessorSpec.h

Lines 130 to 146 in 3d5a983

if (mCheckRunStartStop) {

const auto* grp = mRunChecker.check(); // check if there is a run with EMC

// this is an example of what it will return

if (mRunChecker.getRunStatus() == RunStatus::NONE) {

LOGP(info, "No run with is ongoing or finished");

} else if (mRunChecker.getRunStatus() == RunStatus::START) { // saw new run with wanted detectors

LOGP(info, "Run {} has started", mRunChecker.getFollowedRun());

grp->print();

mProcessor->setRunNumberFromGRP(mRunChecker.getFollowedRun());

} else if (mRunChecker.getRunStatus() == RunStatus::ONGOING) { // run which was already seen is still ongoing

LOGP(info, "Run {} is still ongoing", mRunChecker.getFollowedRun());

} else if (mRunChecker.getRunStatus() == RunStatus::STOP) { // run which was already seen was stopped (EOR seen)

LOGP(info, "Run {} was stopped", mRunChecker.getFollowedRun());

}

} else {

mProcessor->setRunNumberFromGRP(-2);

}

shahor02 · 2022-07-21T16:01:30Z

+
+  /// / return timestamp of first fetched datapoint for a given ID (Tin/Tout,
+  /// Environment pressure, HV, chamber pressure)
+  TimeStampType getMinTime(const std::vector<DPCOM> dps)


pass by ref. instead of copy: const std::vector<DPCOM>& dps

shahor02 · 2022-07-21T16:01:38Z

+  }
+  // return timestamp of last fetched datapoint for a given ID (Tin/Tout,
+  // Environment pressure, HV, chamber pressure)
+  TimeStampType getMaxTime(const std::vector<DPCOM> dps)


shahor02 · 2022-07-21T16:01:52Z

+    return lastTime;
+  }
+
+  void checkEntries(const std::vector<TF1> arQthresh,


shahor02 · 2022-07-21T16:03:53Z

+
+    for (int iCh = 0; iCh < 7; ++iCh) {
+      for (int iSec = 0; iSec < 6; ++iSec) {
+        auto tf = arQthresh.at(6 * iCh + iSec);


use [i] instead of .at(i), also below

shahor02 · 2022-07-21T16:10:29Z

+
+  std::unique_ptr<HMPIDDCSProcessor> mProcessor;
+  HighResClock::time_point mTimer;
+  int64_t mDPsUpdateInterval;


I don't see where you set or use update interval. Please avoid very frequent updates of CCDB at every change. See how other detectors, e.g. FIT/FT0/calibration/testWorkflow/FT0DCSDataProcessorSpec.h do

Hi @shahor02, variable mDPsUpdateInterval is not used indeed, is there by mistake, we are going to remove it! We update CCDB just at the end of RUN (see endOfStream method). I think this is correct! At least if I have not misunderstood something!

Hi @gvolpe79
The endOfStream for the DP processor is rather meaningless: the DP processing workflow runs continuously and is not synchronized with SOR/EOR. The only way to detect run start/stop is to use mRunChecker.getRunStatus() as you do and send run-related output from the run(..) method. That said, note that it is not 100% reliable: if the AliECS crashes, the EOR will be never flagged in the CCDB. @knopers8 works on fixing this but the ETA is not clear.

shahor02 · 2022-08-25T20:45:43Z

+  std::unique_ptr<TF1> pEnv = finalizeEnvPressure();
+  for (int iCh = 0; iCh < 7; iCh++) {
+
+    std::unique_ptr<TF1> pChPres = finalizeChPressure(iCh);


What is the role of these histos? They are created and deleted when going out of scope w/o being used

Indeed, are not in the CCDB but used to create the objects (charge threshold) to be stored in the CCDB (see lines 717 and 718 ) and then deleted.

I see that you are using the name only, why do you need to create a histo?

They are not an histos, they are TF1 objects coming from the fit of histos.

OK, still don't fully understand the logic, but given that it is done once per run at finalization, the overhead should not be important.

shahor02 · 2022-08-25T20:49:49Z

+    outputs, AlgorithmSpec{adaptFromTask<o2::hmpid::HMPIDDCSDataProcessor>()},
+    Options{{"ccdb-path",
+             VariantType::String,
+             "localhost:8080",


please use o2::base::NameConf::getCCDBServer() as a default.

Do you mean to replace the local host?

Yes, the default server should be o2::base::NameConf::getCCDBServer(), so it can be globally changed.

ok, we are going to modify the line

shahor02 · 2022-08-25T20:50:56Z

+             {"Use CCDB to configure"}},
+            {"follow-hmpid-run",
+             VariantType::Bool,
+             false,


if I get it right, with default setting (no run following) you will not produce any output? Is this what you want?

Yes, this is what we want.

ok, but then it is not clear why this is an option, moreover set to false by default ?

You are right, the idea was to optimize the code later on! Anyway, since it is simple to modify it, we are doing to do it now.

Giacomo Volpe added 11 commits June 13, 2022 23:37

HMPID condition paramters calibration using DCS DPs

8a68aa3

Fixing convenctions

45a8d37

Fixing conventions

ab9c7b6

Fix tabs

58dd1fd

Fixing spaces

722243c

Spaces

a6c1ba7

Conventions

f266bea

Fix

535f74f

Fixing clang

ba816ea

Fixing calibration/CMakeList.txt

b4ece21

Fixing calibration/macros/CMakeList.txt

548e9ac

shahor02 reviewed Jun 21, 2022

View reviewed changes

Giacomo Volpe added 4 commits June 29, 2022 12:09

Run dependence implemented

08db3ca

Fixing copyright headers

d1936b3

Convection

e871730

Copyright headers

763b89c

shahor02 requested changes Jul 21, 2022

View reviewed changes

gvolpe79 and others added 2 commits August 22, 2022 15:14

Merge branch 'AliceO2Group:dev' into hmpcalib

5f89766

Minors

3256e10

shahor02 reviewed Aug 25, 2022

View reviewed changes

gvolpe79 and others added 4 commits August 26, 2022 10:23

Merge branch 'AliceO2Group:dev' into hmpcalib

1503cf1

Updtate to global CCDB server and following run as default option

117522d

Fixing copyright

695b2cc

Fixing clang

0bddbf3

shahor02 approved these changes Sep 1, 2022

View reviewed changes

shahor02 merged commit 8302be8 into AliceO2Group:dev Sep 1, 2022

gvolpe79 deleted the hmpcalib branch December 19, 2022 18:54

	if (mCheckRunStartStop) {
	const auto* grp = mRunChecker.check(); // check if there is a run with EMC
	// this is an example of what it will return
	if (mRunChecker.getRunStatus() == RunStatus::NONE) {
	LOGP(info, "No run with is ongoing or finished");
	} else if (mRunChecker.getRunStatus() == RunStatus::START) { // saw new run with wanted detectors
	LOGP(info, "Run {} has started", mRunChecker.getFollowedRun());
	grp->print();
	mProcessor->setRunNumberFromGRP(mRunChecker.getFollowedRun());
	} else if (mRunChecker.getRunStatus() == RunStatus::ONGOING) { // run which was already seen is still ongoing
	LOGP(info, "Run {} is still ongoing", mRunChecker.getFollowedRun());
	} else if (mRunChecker.getRunStatus() == RunStatus::STOP) { // run which was already seen was stopped (EOR seen)
	LOGP(info, "Run {} was stopped", mRunChecker.getFollowedRun());
	}
	} else {
	mProcessor->setRunNumberFromGRP(-2);
	}

Conversation

gvolpe79 commented Jun 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants