refactor(sapere): simplify execution logic in SAPEREReaction#5317
refactor(sapere): simplify execution logic in SAPEREReaction#5317
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors SAPEREReaction.execute() to simplify match selection/execution flow by extracting helper methods, aiming to make the SAPERE reaction execution logic easier to follow and maintain.
Changes:
- Extracted action execution and molecule removal into dedicated helper methods.
- Centralized match selection logic into
selectMatchIndex()/selectWeightedMatchIndex(). - Moved the “empty execution” optimization into
updateEmptyExecutionStatus(...).
| final double selectedPropensity = rng.nextDouble() * totalPropensity; | ||
| double cumulativePropensity = 0; | ||
| for (int i = 0; i < propensities.size(); i++) { | ||
| cumulativePropensity += propensities.get(i); | ||
| if (cumulativePropensity > selectedPropensity) { | ||
| return i; | ||
| } | ||
| } | ||
| throw new IllegalStateException("No match selected despite positive total propensity."); |
There was a problem hiding this comment.
selectedPropensity = rng.nextDouble() * totalPropensity can round up to exactly totalPropensity for large values (even though nextDouble() is < 1.0), and the strict > comparison can then fail to select any index and throw. Consider clamping selectedPropensity to < totalPropensity (e.g., Math.nextDown(totalPropensity)) and/or ensuring the last bucket is always selected (e.g., return the last index after the loop or use >= on the final iteration).
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Fixed in 39a31ee. The throw is now replaced with return propensities.size() - 1 so the last bucket is always selected when floating-point rounding causes selectedPropensity to equal totalPropensity.
DanySK
force-pushed
the
refactor/sapere/simplifyexecutionlogicin
branch
2 times, most recently
from
a689575 to
34b5e9c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5317 +/- ##
=========================================
Coverage 61.53% 61.53%
Complexity 14 14
=========================================
Files 2 2
Lines 78 78
Branches 4 4
=========================================
Hits 48 48
Misses 24 24
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DanySK
force-pushed
the
refactor/sapere/simplifyexecutionlogicin
branch
8 times, most recently
from
9a5d5b0 to
4571ded
Compare
Head branch was pushed to by a user without write access
DanySK
force-pushed
the
refactor/sapere/simplifyexecutionlogicin
branch
2 times, most recently
from
b85758d to
2365dba
Compare
…model/sapere/reactions/SAPEREReaction.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/AlchemistSimulator/Alchemist/sessions/7e9f52ca-8177-4154-90be-f042f4b14d65 Co-authored-by: DanySK <1991673+DanySK@users.noreply.github.com>
DanySK
force-pushed
the
refactor/sapere/simplifyexecutionlogicin
branch
from
2365dba to
6f31ac8
Compare
|
No description provided.