Pawel plesniak/absorber shift - #1875
Conversation
|
Hi @PawelPlesniak,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for 3bb3d47: build (Build queue - API unavailable) |
|
…e really should not just ignore the pre-commit config
|
I leave this for review by @YongyiBWu and @AndrewEdmonds11 |
|
Note - I will review any failures that the CI workflow raises |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for b19652b: build (Build queue - API unavailable) |
|
☔ The build is failing at b19652b.
N.B. These results were obtained from a build of this Pull Request at b19652b after being merged into the base branch at 8a343bd. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 7463724: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 7463724.
N.B. These results were obtained from a build of this Pull Request at 7463724 after being merged into the base branch at 8a343bd. For more information, please check the job page here. |
|
@YongyiBWu and @AndrewEdmonds11 The CI workflow issues have been addressed, this is now ready for review |
🔴 Bugs (likely functional problems)1. if (!stepHandle || stepHandle->empty()) { consecutiveEmptyFileCounter++; return; } // returns BEFORE threshold check
...
if (consecutiveEmptyFileCounter > consecutiveEmptyFileThreshold)
throw cet::exception("LogicError", "Too many consecutive empty files, stopping the job");Because the empty branches 2. ADC = _chargeDecayed[i] * chargeToADC;
_adcs[i] = (ADC > ADCMax) ? static_cast<int16_t>(std::round(ADC)) : ADCMax;
3. if (defaultMicrospillBufferLengthCount < 1)
throw cet::exception("RANGE", "defaultMicrospillBufferLengthCount has to be at least 1!\n");
4. if (E < std::numeric_limits<double>::epsilon())
throw cet::exception("RANGE") << "Energy must be greater than zero, exiting.";
5. With no 🟠 Correctness / robustness concerns6. Uninitialized counters read in summaries — In both 7. 8. Hard-coded 320 MHz coupling — 🟡 Minor / cleanup
|
|
📝 The HEAD of |
YongyiBWu
left a comment
YongyiBWu
left a comment
There was a problem hiding this comment.
I do not see any direct issue with the proposed changes
AndrewEdmonds11
left a comment
AndrewEdmonds11
left a comment
There was a problem hiding this comment.
This looks good to me. There are several files which just have whitespace changes, which would be good to clean up before merging. There are some very useful modules in here that would be good for generating STM mock datasets: PhotonGun, HPGeWaveformsFromStepPoints etc.
There was a problem hiding this comment.
This looks like it's only whitespace changes. I think this file also got renamed at some point so might cause a merge issue...
There was a problem hiding this comment.
I'll merge develop again before I merge, but I have a quick question - for the whitespace changes, what do you mean by "clean up before merging"? It took a while to be able to commit as the pre-commit action I had blocked the git commit before these were all changed, and I am unsure of how to disable this so I can re-commit the files with the whitespaces addressed
Responses to AI review🔴 Bugs (likely functional problems)1. SimParticleDump_module.cc — empty-file counter never resets, and threshold check is wrong
This has been extended to check that the threshold is exceeded before checking that the event data products are not empty.
This has been updated to unify the logic
Validation complete. HPGeWaveformsFromStepPointMCs_module.cc — digitize() clipping logic is invertedThis review is incomplete, and does not understand the physics of what is being simulated:
Positive overflow will not be encountered, unless the user sets the pedestal too high. This will be left for the user to address 3. HPGeWaveformsFromStepPointMCs_module.cc — validation constant defaultMicrospillBufferLengthCount < 1 is unreachableFair point, corrected. 4. PhotonGun_module.cc — new energy check rejects valid low-energy photonsCorrected. 5. HPGeWaveformGenerationAndAnalysis.fcl — dangling data-product tag after module renameGood catch! 🟠 Correctness / robustness concerns6. Uninitialized counters read in summariesCorrected. 7. SimParticleDump uses SimParticles.at(step.trackId())Corrected 8. Hard-coded 320 MHz couplingThe user is made aware of the difference if 🟡 Minor / cleanupTypo: unqiueEnabledVDsVec in CountVirtualDetectorHits_module.cc, and duplicate ;;Corrected Debug leftoversRemoved Copy-paste in .fclCorrected Author-acknowledged unfinished workThis is actually for the user to populate. Comment typo:Addressed where relevant |
|
I will wait for the build to be completed, review any issues should they arise, and await clarification on what the whitespace changes should be |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 265298f: build (Build queue - API unavailable) |
5 participants
This PR addresses several issues found when completing the corrections to my normalization studies.
Changes are summarized in the bullet point list for each file affected by core code changes. Critically note - there are several other files that are also affected, but the
pre-commitconfig highlighted many changes that needed to happen before my changes could go in (after rebasing with branchmain). These changes have been addressed.CountVirtualDetectorHits_module.cc- this makes the vector of virtual detectors being summarized in the model unique, my mapping to and back from an unordered setPhotonGun_module.cc- this now allows for a set of photons to travel in a straight line from the origin to target position, defined by a direction vector asdelta_x,delta_y, anddelta_z. This vector is mapped to the unit vector, and the momentum is scaled accordingly.AbsorberFromSTHPGe.fcl- driver code that will allow future unfolding studies to be performed, especially important for the HPGe where additional absorber is present, and the results have not yet been fully quantified.AbsorberFromSTLaBr.fcl- as above but for LaBr.EventFilter.fcl- debugging tool to help with ongoing simulation studies. Allows for looking at individual events in a data file. For my studies, it was used to demonstrate the behaviour of the ZS and MWD algorithms on a macro-spill worth of beam-on time.FirePhotonToDetector.fcl- driver code that fires singular photons to a detector of choice, aimed for use with studying the STM detector waveform generators and analysis tools.STMMC/fcl/prolog.fcl- rewritten to define the simulation parameters more clearlyROOTAnalysisDump.fcl- driver code to convert the art files into ROOT format for easy plotting. I will not be rewriting this for the current analysis framework as I do not have the time for this.HPGeWaveformsFromStepPointMCs_module.cc- addresses issues with the waveform generator, namelySimParticleAndVDBacktrace_module.cc- new module generating ROOT files with data that backtraces of STM signal photons back through theSimParticleprovenance. The code to plot these will be merged in the STMAnalysis repositorySimParticleDump_module.cc- dumps the fullSimParticleprovenance to ROOT files for plotting, with a more compact format than the previous module.STMResamplingProducer_module.cc- Initial simulations had different data product names forSimParticles propagated with theBeamToVDandBeamToVD1809drivers, this change standardizes them.VirtualDetectorTree_module.cc- add more data to ROOT files generated for plotting.STMMovingWindowDeconvolution_module.cc- some more data for visualizing the behaviour of analyzed waveformsI would like to ask some questions
STMMWDDigirenamed toSTMPHDigi, and what is the difference in analysis chains?