Skip to content

Comments

Fixes #2760 Add validation of Nan and Inf parameters at t=0#2761

Merged
rwmcintosh merged 6 commits intodevelopfrom
2760-add-validation-of-nan-and-inf-parameters-at-t0
Feb 2, 2026
Merged

Fixes #2760 Add validation of Nan and Inf parameters at t=0#2761
rwmcintosh merged 6 commits intodevelopfrom
2760-add-validation-of-nan-and-inf-parameters-at-t0

Conversation

@rwmcintosh
Copy link
Member

@rwmcintosh rwmcintosh commented Feb 2, 2026

Fixes #2760 Add validation of Nan and Inf parameters at t=0

Description

Introducing a new user setting and a task to create validation messages for simulation creation and for simulation runtime.

Type of change

Please mark relevant options with an x in the brackets.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires documentation changes (link at least one user or developer documentation issue):
  • Algorithm update - updates algorithm documentation/questions/answers etc.
  • Other (please describe):

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

  • Integration tests
  • Unit tests
  • Manual tests
  • No tests required

Reviewer checklist

Mark everything that needs to be checked before merging the PR.

  • Check if the code is well documented
  • Check if the behavior is what is expected
  • Check if the code is well tested
  • Check if the code is readable and well formatted
  • Additional checks (document below if any)
  • Check if documentation update issue(s) are created if the option This change requires a documentation update above is selected

Screenshots (if appropriate):

Questions (if appropriate):

Summary by CodeRabbit

  • New Features

    • Warnings for non-finite values (NaN/Infinity) during model creation and simulation runs, including optimized/removed local parameters.
    • New user setting to enable/disable non-finite quantity warnings.
    • Improved user-facing messages for parameters/quantities that are NaN or Infinity at time zero.
  • Tests

    • Comprehensive unit tests covering warning behavior and messaging scenarios.

@rwmcintosh rwmcintosh self-assigned this Feb 2, 2026
@rwmcintosh rwmcintosh added this to V12.3 Feb 2, 2026
@coderabbitai
Copy link

coderabbitai bot commented Feb 2, 2026

📝 Walkthrough

Walkthrough

Adds validation and warnings for non-finite (NaN/Infinity) values at time zero, integrates checks into model creation and run flows, introduces WithValidationResult/CreationResult/RunValidationResult types, a SimulationQuantityValueWarningTask service, a user setting toggle, and unit tests.

Changes

Cohort / File(s) Summary
Error Messaging
src/OSPSuite.Assets/UIConstants.cs
Added three public message strings: RemovedParameterDueToNanAtTimeZero, QuantityIsNanAtTimeZero, QuantityIsInfinityAtTimeZero. Minor whitespace/formatting tweaks.
Domain Validation Types
src/OSPSuite.Core/Domain/CreationResult.cs
Added WithValidationResult base exposing ValidationResult; added CreationResult : WithValidationResult (re-exposes IModel and SimulationBuilder) and RunValidationResult : WithValidationResult; moved Deconstruct and defaulted ValidationResult initialization.
Warning Service
src/OSPSuite.Core/Domain/Services/SimulationQuantityValueWarningTask.cs
New ISimulationQuantityValueWarningTask and SimulationQuantityValueWarningTask implementation that enumerates NaN/Infinity quantities and optimized/removed parameters and appends ValidationMessage warnings for creation and run contexts.
Model Construction Integration
src/OSPSuite.Core/Domain/Services/ModelConstructor.cs
Injected ISimulationQuantityValueWarningTask; updated finalizeModel and removeUndefinedLocalMoleculeParametersIn to accept CreationResult and invoke warning methods during finalization.
User Settings
src/OSPSuite.Core/ICoreUserSettings.cs, tests/OSPSuite.Starter/Services/CoreUserSettings.cs
Added bool WarnForNonFiniteQuantities { get; set; } to toggle generation of non-finite value warnings; test settings mirrored.
Tests
tests/OSPSuite.Core.Tests/Services/SimulationQuantityValueWarningTaskSpecs.cs
Added extensive unit tests covering NaN/Infinity warning generation across creation/run contexts, optimized/removed parameters, and user-setting toggles.
Misc / Solution
CLAUDE.md, OSPSuite.Core.sln
Minor CLAUDE.md bracing rule edit and added CLAUDE.md to solution items.

Sequence Diagram(s)

sequenceDiagram
    participant Consumer as Consumer (create/run)
    participant MC as ModelConstructor
    participant CR as CreationResult / RunValidationResult
    participant SQV as SimulationQuantityValueWarningTask
    participant M as IModel

    Consumer->>MC: request create/run (model, builder)
    MC->>CR: construct CreationResult / RunValidationResult
    MC->>MC: finalizeModel(model, simulationBuilder, creationResult)
    MC->>SQV: WarnForNonFiniteQuantities(model, creationResult)
    SQV->>M: enumerate parameters & molecule amounts
    SQV->>SQV: detect NaN / Infinity values
    SQV->>CR: append ValidationMessage warnings
    MC->>SQV: WarnForOptimizedLocalMoleculeParameters(optimizedParams, creationResult)
    SQV->>CR: append parameter-specific warnings
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I sniffed the numbers at break of day,
Found NaN and Inf that wanted to play,
I left a small note where builders roam,
So creations warn before they come home,
Hooray — safer simulations all the way!

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive The PR includes changes to CLAUDE.md and OSPSuite.Core.sln that appear unrelated to the NaN/Inf validation feature requirements. Clarify whether CLAUDE.md style rule updates and solution file changes are intentional scope or should be reverted to a separate PR.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding validation for NaN and Inf parameters at time zero, matching the linked issue #2760.
Linked Issues check ✅ Passed The PR fully implements the linked issue #2760 objectives: detects non-finite parameters at t=0 and generates validation messages during creation and runtime.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 2760-add-validation-of-nan-and-inf-parameters-at-t0

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/OSPSuite.Assets/UIConstants.cs (1)

1589-1594: ⚠️ Potential issue | 🟡 Minor

Add braces around the conditional.
This file uses the no-brace pattern here, but the guideline requires braces for conditionals.

Suggested fix
-         if (string.IsNullOrEmpty(containerType))
-            return NameAlreadyExists(name);
+         if (string.IsNullOrEmpty(containerType))
+         {
+            return NameAlreadyExists(name);
+         }

As per coding guidelines, Always use braces for loops and conditionals.

🤖 Fix all issues with AI agents
In `@src/OSPSuite.Core/Domain/Services/SimulationQuantityValueWarningTask.cs`:
- Around line 44-67: Several if statements lack braces (violating style rules) —
add explicit braces to all conditional blocks in this file: wrap the bodies of
the if (!_userSettings.WarnForNonFiniteQuantities) checks found in
WarnForOptimizedLocalMoleculeParameters, WarnForNonFiniteQuantities, and
warnForNonFiniteQuantities with { ... }, and likewise add braces to the other
brace-free conditionals noted around the later block (the ones referenced by the
reviewer at the 92-96 region). Ensure each conditional uses braces even for
single-line returns or single statements to conform to the coding guideline.

In
`@tests/OSPSuite.Core.Tests/Services/SimulationQuantityValueWarningTaskSpecs.cs`:
- Around line 13-353: The tests repeat literal IDs and names for NaN quantities;
add constants in the base test class concern_for_SimulationQuantityWarningTask
(e.g., NAN_AMOUNT_ID, NAN_AMOUNT_NAME, NAN_PARAMETER_ID, NAN_PARAMETER_NAME,
DEFAULT_BUILDER_NAME) and replace all hard-coded strings used in
NewSimulationQuantity()/NewBuilder() and the setup (calls to WithId/WithName and
AddSimulationEntitySource) with those constants so all scenarios reuse the
shared identifiers.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/OSPSuite.Core/Domain/Services/SimulationQuantityValueWarningTask.cs`:
- Around line 44-50: The method WarnForOptimizedLocalMoleculeParameters
unconditionally adds warnings for every optimizedParameters entry; update it to
respect the SimulationBuilder's WarnForNonFiniteQuantities flag by checking
creationResult.SimulationBuilder.WarnForNonFiniteQuantities (null-safe) and
returning early when the flag is false so no ValidationResult/ValidationMessage
(created via Warning.RemovedParameterDueToNanAtTimeZero, ValidationResult,
builderAndBuildingBlockFor, etc.) are added; ensure you still use the same
builderAndBuildingBlockFor(...) logic when the flag is true.

@rwmcintosh rwmcintosh merged commit 01a7814 into develop Feb 2, 2026
6 checks passed
@rwmcintosh rwmcintosh deleted the 2760-add-validation-of-nan-and-inf-parameters-at-t0 branch February 2, 2026 18:20
@github-project-automation github-project-automation bot moved this to Done in V12.3 Feb 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Add validation of Nan and Inf parameters at t=0

2 participants