share user vars - #143
Conversation
…esult of resettting parameters, as is done when multiple random fits are performed in sequence
…ss AmpVecs objects
…iable cache tag; this enhances safety
…terms are deallocated; also handle maintaining the links to user variables amongst shared data after the four vectors are flushed
… clearning the terms
…tic and nonstatic data but just modify the tag; more stringent error checking for the offset lookup
…during cache update; this allows all the links to be formed between shared data objects
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in the new user-var keying/validity flow (permutation-tag collisions and missing/ineffective invalidation/retention paths) that can lead to incorrect reuse or runtime failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces infrastructure to reuse/share computed per-event “user variables” across reactions that share the same underlying MC datasets, reducing repeated computation (notably for workflows where the same accepted MC is reused across multiple reactions).
Changes:
- Makes
NormIntInterface::loadMC()virtual and calls it explicitly during configuration so shared MC datasets can be linked earlier (with an MPI-specific override to avoid leader allocations). - Adds user-variable validity tracking and user-var sharing lookup (
AmpVecs::findSharedUserVars) so previously computed user-vars can be reused/copied across shared-data “friends”. - Updates allocation/deallocation paths (CPU/GPU) to optionally retain cached user-vars across some term deallocation cycles.
File summaries
| File | Description |
|---|---|
| AmpTools/IUAmpToolsMPI/NormIntInterfaceMPI.h | Declares MPI-specific loadMC() override. |
| AmpTools/IUAmpToolsMPI/NormIntInterfaceMPI.cc | Implements follower-only loadMC() to avoid leader memory pressure. |
| AmpTools/IUAmpTools/NormIntInterface.h | Makes loadMC() virtual for MPI override. |
| AmpTools/IUAmpTools/NormIntInterface.cc | Triggers loadMC() on-demand; retains gen-MC user-vars across deallocTerms(false). |
| AmpTools/IUAmpTools/AmpVecs.h | Adds m_userVarsValid, extends deallocTerms/clearFourVecs, declares findSharedUserVars. |
| AmpTools/IUAmpTools/AmpVecs.cc | Implements user-var retention logic, sharing lookup, and modified sharing/ownership transitions. |
| AmpTools/IUAmpTools/AmpToolsInterface.cc | Calls NormIntInterface::loadMC() explicitly after construction. |
| AmpTools/IUAmpTools/AmplitudeManager.h | Adds getPermutationTag helper. |
| AmpTools/IUAmpTools/AmplitudeManager.cc | Keys user-var storage by (amp id + permutation tag) and reuses shared user-vars when possible. |
| AmpTools/GPUManager/GPUManager.h | Adds clearTerms(bool clearUserVars) API. |
| AmpTools/GPUManager/GPUManager.cc | Implements conditional freeing of device user-vars in clearTerms. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
who is this harsh reviewer? :D Hi Matt! |
There was a problem hiding this comment.
🟡 Changes recommended
The current implementation has confirmed correctness/compatibility issues (permutation-tag collisions, forceUserVarRecalculation not reliably forcing recomputation, and an unguarded loadMC() call that can break __ACLIC__ builds).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
AmpTools/IUAmpTools/AmplitudeManager.cc:427
setForceUserVarRecalculation(true)is documented to force user-variable recomputation every timecalcTermsis called, butcalcTermscurrently only callscalcUserVarswhen!a.m_userVarsValid. If user vars are already valid, the force flag will be ignored.
if( !a.m_userVarsValid && a.m_userVarsPerEvent > 0 && startEvent == 0 ){
AmpTools/IUAmpTools/AmplitudeManager.cc:1349
getPermutationTagconcatenates permutation indices without separators (e.g.,[1,11]and[11,1]both serialize as111), which can cause collisions in the ampId key used for user-var storage/sharing and lead to incorrect reuse of user vars. Add an unambiguous delimiter between indices.
sig << "|";
for( vector< int >::const_iterator itr = vecItr->begin();
itr != vecItr->end(); ++itr ){
sig << *itr;
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
This adds logic to share user variables that are computed from shared MC sets. For some applications (GlueX) using the same accepted MC for different reactions is common and this sharing optimizes compute time. This change also avoids clearing the user variables when parameters are reset because the user variables should not depend on floating parameters in the fit.