MPI comments - #144
Conversation
…ns regarding loadMC()
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a breaking API/ABI change and removes an MPI safety override that can lead to unintended MC loading on the leader rank when called via a base pointer.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR is intended to clarify (via comments) how Monte Carlo loading differs between MPI and non-MPI normalization-integral implementations, particularly around when loadMC() is invoked.
Changes:
- Added comments in MPI setup code and MPI interface construction to explain follower-only MC loading.
- Removed the MPI-specific
loadMC()override and its declaration. - Changed
NormIntInterface::loadMC()fromvirtualto non-virtual (public API/ABI change).
File summaries
| File | Description |
|---|---|
| AmpTools/IUAmpToolsMPI/NormIntInterfaceMPI.h | Removes the loadMC() declaration from the MPI-derived interface. |
| AmpTools/IUAmpToolsMPI/NormIntInterfaceMPI.cc | Adds explanatory comments in setupMPI() and removes the MPI loadMC() implementation. |
| AmpTools/IUAmpToolsMPI/AmpToolsInterfaceMPI.cc | Adds a comment noting loadMC() is invoked via NormIntInterfaceMPI construction/setup. |
| AmpTools/IUAmpTools/NormIntInterface.h | Changes loadMC() from virtual to non-virtual, affecting extensibility and ABI. |
Review details
Suppressed comments (1)
AmpTools/IUAmpToolsMPI/NormIntInterfaceMPI.cc:207
- The MPI-specific
loadMC()implementation was removed. Without it (and withoutloadMC()being virtual in the base), code that callsloadMC()on the leader rank via aNormIntInterface*can trigger large, unnecessary allocations. Restore the MPI override that no-ops on the leader.
delete[] result;
}
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // needs different implementations in MPI so make virtual | ||
| virtual void loadMC() const; | ||
| void loadMC() const; |
…s on the lead process
There was a problem hiding this comment.
🟡 Changes recommended
The PR’s new explanatory comments include a misleading statement about when loadMC() runs (leader vs follower ranks), and there are minor formatting/trailing-whitespace issues that should be corrected since the PR is documentation-focused.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 2
- Review effort level: Lite
| // no need to call loadMC() here because the constructor of | ||
| // NormIntInterfaceMPI calls it through setupMPI() |
| void | ||
| NormIntInterfaceMPI::loadMC() const | ||
| { | ||
| // avoids large memory allocations on the lead node | ||
| NormIntInterfaceMPI::loadMC() const{ | ||
|
|
||
| // do not load MC on the leader node | ||
| // only the followers need the MC |
Add some comments to explain differences in MPI and standard implementations and the need for loadMC