Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new boolean AutoReduceTolerances option across the simulation stack (Core and R), propagating the setting from SimulationRunOptions through SimulationRunner/SimModelManager and SimModelBatch into the created Simulation; defaults are enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant R as "R caller"
participant Runner as "SimulationRunner / SimModelManager"
participant Batch as "SimModelBatch"
participant Simulation as "Simulation"
R->>Runner: provide SimulationRunOptions (AutoReduceTolerances=true)
Runner->>Batch: Create/Initialize batch (set AutoReduceTolerances)
Batch->>Simulation: Create Simulation with Options (AutoReduceTolerances set)
Simulation-->>Runner: Run result (uses AutoReduceTolerances when handling failures)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OSPSuite.R/Services/SimulationRunner.cs (1)
85-108:⚠️ Potential issue | 🟠 MajorThe population runner does not propagate
AutoReduceTolerancesto individual simulations.The population path (lines 96–101) passes the R-domain
SimulationRunOptionsto_populationRunner.RunPopulationAsync, which receives it as aRunOptionsparameter. However,PopulationRunneronly extractsNumberOfCoresToUsefrom the options and creates individual simulations viaCreateSimulation(simulationExport)without passing any configuration action. This meansAutoReduceTolerancesis never applied to the SimModel options for population runs, unlike the individual simulation path (line 120) which explicitly sets it viaSimModelManager.simulate().To fix this,
PopulationRunner.RunPopulationAsyncneeds to acceptSimulationRunOptionsinstead of the baseRunOptionstype, and pass a configuration action toCreateSimulationthat setsAutoReduceTolerancesandCheckForNegativeValueson the simulation options (similar to howSimModelManager.simulate()does at lines 98–99).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OSPSuite.R/Services/SimulationRunner.cs` around lines 85 - 108, The population path currently calls _populationRunner.RunPopulationAsync with a SimulationRunOptions but PopulationRunner only accepts RunOptions and doesn't forward AutoReduceTolerances/CheckForNegativeValues to individual simulations; change PopulationRunner.RunPopulationAsync to accept SimulationRunOptions (not RunOptions) and update its internal CreateSimulation calls to pass a configuration action that sets sim.Options.AutoReduceTolerances and sim.Options.CheckForNegativeValues from the incoming SimulationRunOptions (mirroring how SimModelManager.simulate applies these) so population-run individual simulations receive the same tolerance/negative-value settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/OSPSuite.R/Services/SimulationRunner.cs`:
- Around line 85-108: The population path currently calls
_populationRunner.RunPopulationAsync with a SimulationRunOptions but
PopulationRunner only accepts RunOptions and doesn't forward
AutoReduceTolerances/CheckForNegativeValues to individual simulations; change
PopulationRunner.RunPopulationAsync to accept SimulationRunOptions (not
RunOptions) and update its internal CreateSimulation calls to pass a
configuration action that sets sim.Options.AutoReduceTolerances and
sim.Options.CheckForNegativeValues from the incoming SimulationRunOptions
(mirroring how SimModelManager.simulate applies these) so population-run
individual simulations receive the same tolerance/negative-value settings.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/OSPSuite.Core/Domain/Services/ISimModelBatch.cssrc/OSPSuite.Core/Domain/Services/SimModelBatch.cssrc/OSPSuite.Core/Domain/Services/SimModelManager.cssrc/OSPSuite.Core/Domain/SimulationRunOptions.cssrc/OSPSuite.R/Domain/SimulationBatch.cssrc/OSPSuite.R/Domain/SimulationRunOptions.cssrc/OSPSuite.R/Services/SimulationRunner.cstests/OSPSuite.Core.IntegrationTests/SimModelBatchSpecs.cstests/OSPSuite.Core.Tests/Domain/SimulationRunOptionsSpecs.cs
| public void Initialize(IModelCoreSimulation simulation, SimulationBatchOptions simulationBatchOptions, SimulationRunOptions simulationRunOptions) | ||
| { | ||
| _simModelBatch.CheckForNegativeValues = simulationRunOptions?.CheckForNegativeValues ?? true; | ||
| _simModelBatch.AutoReduceTolerances = simulationRunOptions?.AutoReduceTolerances ?? false; |
There was a problem hiding this comment.
The default setting in SimModel is AutoReduceTolerances = true
https://github.com/Open-Systems-Pharmacology/OSPSuite.SimModel/blob/8071fcbb06efe6075b215489c94b6a9228d29f4a/src/OSPSuite.SimModelNative/src/SimulationOptions.cpp#L14C3-L14C23
If the AutoReduceTolerances setting was not modified by OSPSuite.Core till now, then the default is changed to false here for the case simulationRunOptions=null.
Why?
There was a problem hiding this comment.
Did not know about the default sorry, will change it right away.
There was a problem hiding this comment.
I was about to say. Is this the right default?
| /// <summary> | ||
| /// Specifies whether the solver should automatically reduce tolerances when a simulation run fails. Default is <c>false</c> | ||
| /// </summary> | ||
| public bool AutoReduceTolerances { get; set; } = false; |
There was a problem hiding this comment.
Unify syntax please. Should be true by default in code
There was a problem hiding this comment.
I could, but as I can see the other values are set on the constructor instead, I was responsible for the last 2:
public SimulationRunOptions()
{
SimModelExportMode = SimModelExportMode.Full;
CheckForNegativeValues = true;
AutoReduceTolerances = true;
}
But the first has been there before, shallI I completely get rid of the constructor here then and set the 3 properties by default?
msevestre
left a comment
msevestre
left a comment
There was a problem hiding this comment.
Should be set to true and not to false as defau;t
|
@msevestre can you please re-review? |
3 participants
Fixes #1717
Description
Allow auto reduce tolerance
Type of change
Please mark relevant options with an
xin the brackets.How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Reviewer checklist
Mark everything that needs to be checked before merging the PR.
This change requires a documentation updateabove is selectedScreenshots (if appropriate):
Questions (if appropriate):
Summary by CodeRabbit
New Features
Tests