TOF: add trigger efficiency by ercolessi · Pull Request #733 · AliceO2Group/QualityControl

ercolessi · 2021-06-17T15:21:21Z

njacazio · 2021-06-17T20:08:00Z

Ciao @ercolessi great work as usual. I have few very minor comments to improve the code but other than that outstanding job!
If you can consider my suggestions I would move this to ready for review from the QC team

njacazio · 2021-06-17T19:56:25Z

  static const char* DRMDiagnosticName[nwords]; /// DRM Counter names
  static const char* LTMDiagnosticName[nwords]; /// LTM Counter names
  static const char* TRMDiagnosticName[nwords]; /// TRM Counter names
  // Diagnostic counters


Great job, what about defining a constexpr counter for the RDH names?

njacazio · 2021-06-17T19:57:30Z


  // Names of diagnostic counters
-  static const char* RDHDiagnosticsName[2];     /// RDH Counter names
+  static const char* RDHDiagnosticsName[3];     /// RDH Counter names


something like static constexpr unsigned int nRDHwords = 3

njacazio · 2021-06-17T19:58:42Z

  Counter<nequipments, nullptr> mCounterNoisyChannels;    /// Counter for noisy channels
  Counter<1024, nullptr> mCounterTimeBC;                  /// Counter for the Bunch Crossing Time
  Counter<91, nullptr> mCounterNoiseMap[ncrates][4];      /// Counter for the Noise Hit Map
+  Counter<ncrates, nullptr> mCounterRDHTriggers[2];       /// Counter for RDH served and received triggers


Perfect! Maybe in the comment you could say why we need 2 of these

njacazio · 2021-06-17T20:00:44Z

@@ -83,6 +83,7 @@ class RawDataDecoder final : public DecoderBase
  Counter<nequipments, nullptr> mCounterNoisyChannels;    /// Counter for noisy channels
  Counter<1024, nullptr> mCounterTimeBC;                  /// Counter for the Bunch Crossing Time
  Counter<91, nullptr> mCounterNoiseMap[ncrates][4];      /// Counter for the Noise Hit Map


If you feel particularly majestic you could add in the comment that we have one per crate and one per FEA (the documentation will pay off in the future!)

njacazio · 2021-06-17T20:01:42Z

  std::shared_ptr<TH1F> mHistoIndexEO;          /// Index in electronic
  std::shared_ptr<TH1F> mHistoIndexEOInTimeWin; /// Index in electronic for noise analysis
  std::shared_ptr<TH1F> mHistoIndexEOIsNoise;   /// Noise hit map x channel
+  std::shared_ptr<TH1F> mHistoRDHTriggers;      /// RDH served and received trigger efficiency


Does this have also the trigger efficiency? The comment is not so clear I fear

njacazio · 2021-06-17T20:02:38Z

 {
  mCounterRDH[rdh->feeId & 0xFF].Count(0);

+  //Fatal


Maybe here it is worth to extend a bit the comment, fatal is a bit too harsh maybe and we can consider printing warning messages, what do you think?

Ciao @njacazio, many thanks for the comments!
So, fatal is how Roberto refers to it in AliceO2Group/AliceO2@90c8e48 .
How would you suggest to change it?

For the warning: sounds great, where you thinking of a LOG(WARNING) or using a checker?

I mean, I don't think we want to throw a fatal error if we encounter this word here, therefore I would at least call it Case for the RDH word: fatal or something that to explain that it is not an error in our task.
I guess you can put the LOG and we will implement the checker as well to inform the QC crew.
Does it seem reasonable?

Everything clear, sounds perfect! I'm on it

njacazio · 2021-06-17T20:02:55Z

+
+  //Trigger expected and served
+  if (rdh->stop) { // if RDH close
+    int triggerserved = ((rdh->detectorField & 0xFF00070F) >> 24);


let's const these bad boys

njacazio · 2021-06-17T20:03:54Z

+      //Trigger error
+      mCounterRDH[rdh->feeId & 0xFF].Count(2);
+    }
+    //Trigger efficiency


This is not strictly the trigger efficiency, maybe you can consider adding: Ingredients for the trigger efficiency (also for the future)

njacazio · 2021-06-17T20:06:24Z

    }
    mHistoSlotParticipating->SetBinContent(crate + 1, 1, mDecoderRaw.mCounterRDH[crate].HowMany(0));
+    if (mDecoderRaw.mCounterRDHTriggers[1].HowMany(crate) != 0) {
+      mHistoRDHTriggers->SetBinContent(crate + 1, (double)mDecoderRaw.mCounterRDHTriggers[0].HowMany(crate) / mDecoderRaw.mCounterRDHTriggers[1].HowMany(crate));


maybe consider using the explicitly the cast i.e. static_cast<double>(... ) or float

njacazio · 2021-06-17T20:28:32Z

@Barthelemy @knopers8 now that we have more people working on the TOF QC project could we have some codeowners for the TOF directory so that we are triggered when something is changed ? Is this possible?

- Add protection

njacazio

Ciao Francesca, for me it's top quality. Few suggestions but I would go forth.
Thanks a lot!
Nicolo'

njacazio · 2021-06-23T12:00:38Z

+  static const char* TRMDiagnosticName[nwords];     /// TRM Counter names
  // Diagnostic counters
-  Counter<2, RDHDiagnosticsName> mCounterRDH[ncrates];            /// RDH Counters
+  Counter<3, RDHDiagnosticsName> mCounterRDH[ncrates];            /// RDH Counters


Maybe you could use the new nRDHwords

yes indeed thanks!

njacazio · 2021-06-23T14:12:04Z

 }

-const char* RawDataDecoder::RDHDiagnosticsName[2] = { "RDH_HAS_DATA", "RDH_DECODER_FATAL" };
+const char* RawDataDecoder::RDHDiagnosticsName[3] = { "RDH_HAS_DATA", "RDH_DECODER_FATAL", "RDH_TRIGGER_ERROR" };


Maybe you could use the new nRDHwords

njacazio · 2021-06-23T14:46:33Z

@Barthelemy @knopers8 I think we are done now and are ready to merge as soon as the tests are passed.
Btw, now that we have more people working on the TOF QC project could we have some codeowners for the TOF directory so that we are triggered when something is changed ? Would that be possible?

Barthelemy · 2021-06-24T06:23:08Z

@njacazio I am going to take a quick look and approve it. Concerning CODEOWNERS I think that it is a good idea. I'll look into it (no idea how it works).

Barthelemy · 2021-06-24T06:24:00Z

https://alice.its.cern.ch/jira/browse/QC-605

Barthelemy · 2021-06-24T06:27:25Z

+  //Case for the RDH word "fatal"
  if ((rdh->detectorField & 0x00010000) != 0) {
    mCounterRDH[rdh->feeId & 0xFF].Count(1);
+    LOG(WARNING) << "RDH flag \"fatal\" error occurred in crate " << static_cast<int>(rdh->feeId & 0xFF);


InfoLogger is used in some places and FairLogger in some others. It is ok for me, just raising your awareness about it.

Thanks! We will address it in the next PR

njacazio reviewed Jun 17, 2021

View reviewed changes

ercolessi added 2 commits June 23, 2021 09:49

add trigger efficiency: served vs received

66429ab

Code improvements

bab113e

ercolessi force-pushed the master branch from b33e83c to bab113e Compare June 23, 2021 07:52

Fix type of MO

0f87727

- Add protection

ercolessi changed the title ~~[WIP] TOF: add trigger efficiency~~ TOF: add trigger efficiency Jun 23, 2021

ercolessi marked this pull request as ready for review June 23, 2021 11:58

njacazio approved these changes Jun 23, 2021

View reviewed changes

use constexpr counter for RDH words

030df14

Barthelemy reviewed Jun 24, 2021

View reviewed changes

Barthelemy approved these changes Jun 24, 2021

View reviewed changes

Barthelemy merged commit e3b29da into AliceO2Group:master Jun 24, 2021

Conversation

ercolessi commented Jun 17, 2021

Uh oh!

njacazio commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

njacazio commented Jun 17, 2021

Uh oh!

njacazio left a comment

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!

njacazio commented Jun 23, 2021

Uh oh!

Barthelemy commented Jun 24, 2021

Uh oh!

Barthelemy commented Jun 24, 2021

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!

3 participants

njacazio commented Jun 17, 2021 •

edited

Loading