Add data product to record filtering fraction#1691
Add data product to record filtering fraction#1691brownd1978 wants to merge 7 commits intoMu2e:mainfrom
Conversation
|
Hi @brownd1978,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for 33c5055: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 33c5055.
N.B. These results were obtained from a build of this Pull Request at 33c5055 after being merged into the base branch at 440d6ec. For more information, please check the job page here. |
sophiemiddleton
left a comment
sophiemiddleton
left a comment
There was a problem hiding this comment.
Thanks Dave, this definitely looks useful!
michaelmackenzie
left a comment
michaelmackenzie
left a comment
There was a problem hiding this comment.
I think this looks good and is very useful, thank you for making this update!
Can you please add this data product output to the Trigger/src/PrescaleEvent_module.cc prescale module? This may require a separate object for On-Spill and Off-Spill in this case.
|
I can also make the update to the Trigger prescaler module after this PR if that's easier! |
Can the OnSpill/OffSpill split be handled by having 2 instances of this module in different paths with the appropriate filter before them? |
I'd prefer for now to keep the trigger configuration as it is, without needing to duplicate all paths (or more multiplying if there are any other event modes) in order to have different pre-scales On-Spill and Off-Spill. The Trigger prescaler is designed to be able to handle different pre-scales for different event modes, without duplication. |
|
I think I addressed all the issues. I tested with on OnSpill digitization job (with Production PR 498) and see the new products look as expected. |
|
Example filedumper output: PRINCIPAL TYPE: SubRun |
michaelmackenzie
left a comment
michaelmackenzie
left a comment
There was a problem hiding this comment.
This looks good and I appreciate you adding the ability to add the filter fraction options. I only requested minor edits to preserve the 100% prescale mode used in Online processing
Trigger/src/PrescaleEvent_module.cc
Outdated
| ++_npass; | ||
| ++nevt_[imode]; | ||
| // Apply the prescale | ||
| bool retval = e.event() % mode.prescale_ == 0; |
There was a problem hiding this comment.
We use ps <= 0 as a special case to entirely prescale a path while still loading the modules and having the path in the art::TriggerResults. Therefore, we need to check this before apply the modulo.
There was a problem hiding this comment.
I'm confused about how negative prescales are intended to work. AFAIK
bool retval = e.event() % mode.prescale_ == 0;
will produce the same result for negative or positive prescale values of the same magnitude.
What do you want the code to do?
I honestly dont see a use case here for negative values, espectially since 0 does what you said above, and users might be confused by negative prescales. Making this an unsigned seems cleaner.
There was a problem hiding this comment.
The current functionality is prescale values <= 0 indicate deactivating the path. This is used to disable paths for specific event modes (e.g. On-Spill) while allowing for processing in alternate event modes (e.g. Off-Spill). The current menus all default path prescales to -1 and so we need to protect against this. Also, a prescale of 0 would cause issues here as well as <value> % 0 is undefined:
root [0] 6 % 0
ROOT_prompt_0:1:3: warning: remainder by zero is undefined [-Wdivision-by-zero]
6 % 0
^ ~
(int) 8
Trigger/src/PrescaleEvent_module.cc
Outdated
| bool PrescaleEvent::endSubRun( art::SubRun& subrun ) { | ||
| for (size_t imode = 0; imode < eventMode_.size(); imode++){ | ||
| auto const& mode = eventMode_[imode]; | ||
| double frac = 1.0/double(mode.prescale_); |
There was a problem hiding this comment.
This prescale value could be <= 0, so this should be checked here as well
There was a problem hiding this comment.
I added protection against prescale == 0, I agree that's a legitimate use case.
There was a problem hiding this comment.
I'm not sure I see where this protection is, above it I still see:
double frac = 1.0/double(mode.prescale_);but it should probably be:
const double frac = (mode.prescale_ > 0) ? 1.0/double(mode.prescale_) : 0.;
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 680c5d0: build (Build queue - API unavailable) |
|
☔ The build tests failed for 680c5d0.
N.B. These results were obtained from a build of this Pull Request at 680c5d0 after being merged into the base branch at 4f1ca06. For more information, please check the job page here. |
|
☔ The build tests failed for 680c5d0.
N.B. These results were obtained from a build of this Pull Request at 680c5d0 after being merged into the base branch at 4f1ca06. For more information, please check the job page here. |
michaelmackenzie
left a comment
michaelmackenzie
left a comment
There was a problem hiding this comment.
Thank you for making these updates!
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 3476511: build (Build queue - API unavailable) |
|
☔ The build tests failed for 3476511.
N.B. These results were obtained from a build of this Pull Request at 3476511 after being merged into the base branch at 45ae2d8. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for a14efe1: build (Build queue - API unavailable) |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for a14efe1: build (Build queue - API unavailable) |
|
☀️ The build tests passed at a14efe1.
N.B. These results were obtained from a build of this Pull Request at a14efe1 after being merged into the base branch at 1148372. For more information, please check the job page here. |
Mu2e / Offline / files The method hasNominalValue() returns true when the type is nonominal, which semantically means "no nominal" value. This appears to be inverted — hasNominalValue() should presumably return true when the type is constant (which does have a nominal value), not nonominal. The naming is confusing and the logic seems wrong. It should likely be: C++
Mu2e / Offline / files The enum value nonominal looks like a typo. It should probably be nonnominal (double-n) or noNominal to clearly convey "no nominal" value. As-is, nonominal reads awkwardly and could be misinterpreted.
Mu2e / Offline / files The condition other.nominalFraction() != nominalFraction() performs an exact floating-point equality comparison on double values. This is fragile and may fail unexpectedly due to floating-point precision issues. Consider using an epsilon-based comparison, or comparing the original integer prescale values instead.
Mu2e / Offline / files The chain() method hard-codes nominal_ = -1.0. But a chained filter of two constant-type filters could have a meaningful computed nominal fraction (i.e., the product of the two nominal fractions). This is a missed opportunity for correctness and may confuse downstream consumers who see -1.0 for a chained filter that actually had well-defined inputs.
Mu2e / Offline / files If nPrescale is configured as 0, this will produce inf via division by zero. The commit "Protect against 0 prescale" was mentioned but no guard is visible here — frac_ is computed unconditionally in the initializer list.
Mu2e / Offline / files
Mu2e / Offline / files The printer has a hardcoded filter for "Cosmic" dataset names. This limits the utility of a supposedly general-purpose printer — it won't print sampled filter fractions for any non-Cosmic datasets. This should either iterate all datasets or be configurable.
Mu2e / Offline / files Multiple lines in PrintSubRun() write to std::cout directly instead of using the os parameter. This breaks the output stream abstraction — if a caller passes a different output stream, the sampled filter fraction output will still go to std::cout.
Mu2e / Offline / files The comment // Apply the prescale has different indentation than the surrounding code within the if block. This is a minor style issue but makes the code harder to read.
Mu2e / Offline / files The header includes (C-style string functions like strlen, memcpy) but uses std::string in the PrintHeader method. It should include instead of (or in addition to) . Summary Severity File Issue1 🔴 High FilterFraction.hh hasNominalValue() has inverted logic |
|
📝 The HEAD of |
5 participants
This is intended to record prescale factors, but is more generalized in case there are other uses.