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. |
|
@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. |
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) |
4 participants
This is intended to record prescale factors, but is more generalized in case there are other uses.