Skip to content

Comments

Fixes #2754 Parameter Identification: erroneous behavior of "Transfer results to Simulation" in combination with "Use as factor"#2755

Merged
rwmcintosh merged 1 commit intodevelopfrom
2754-parameter-identification-erroneous-behavior-of-transfer-results-to-simulation-in-combination-with-use-as-factor
Jan 27, 2026
Merged

Fixes #2754 Parameter Identification: erroneous behavior of "Transfer results to Simulation" in combination with "Use as factor"#2755
rwmcintosh merged 1 commit intodevelopfrom
2754-parameter-identification-erroneous-behavior-of-transfer-results-to-simulation-in-combination-with-use-as-factor

Conversation

@rwmcintosh
Copy link
Member

Fixes #2754

Description

When using use-as-factor we are going to capture the value of linked parameters when the PI is started and then use the captured value and factor rather than continuing to apply the factor to the current parameter value.

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):

… results to Simulation" in combination with "Use as factor"
@rwmcintosh rwmcintosh requested review from Copilot and removed request for Copilot January 27, 2026 17:24
@rwmcintosh rwmcintosh self-assigned this Jan 27, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 27, 2026

Greptile Overview

Greptile Summary

This PR fixes issue #2754 where Parameter Identification with "Use as Factor" was incorrectly applying the factor to the current parameter value instead of the initial value.

Key Changes

  • Captures initial parameter values when Parameter Identification starts (before optimization runs)
  • Uses captured values for factor calculation instead of current parameter values when transferring results back to simulations
  • Maintains backward compatibility by falling back to the old method if InitialValue is null (for PI runs from before this fix)
  • Adds serialization support for the new InitialValue property to persist it in saved PI configurations

Technical Implementation

The fix introduces an InitialValue property on ParameterSelection that gets populated via CaptureIdentificationParameterInitialValues() at the start of PI execution. When calculating optimized values with UseAsFactor=true, the code now multiplies the optimal value by the captured initial value rather than the current parameter value, preventing compounding errors.

Testing

  • Updated unit tests verify the new behavior: parameter value is captured at start, then even if changed to 0, the calculation still uses the captured value
  • Test context setup now mirrors production by calling CaptureIdentificationParameterInitialValues()

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is clean, well-tested, maintains backward compatibility, and properly handles the edge case with a fallback. The fix is focused and doesn't introduce unnecessary complexity. All changed files follow C# best practices and the codebase's existing patterns.
  • No files require special attention

Important Files Changed

Filename Overview
src/OSPSuite.Core/Domain/ParameterSelection.cs Added InitialValue property and updated Clone() method to preserve it during cloning
src/OSPSuite.Core/Domain/ParameterIdentifications/IdentificationParameter.cs Modified OptimizedParameterValueFor() to use captured initial values with backward compatibility fallback, added CaptureLinkedParameterValues() method
src/OSPSuite.Core/Domain/Services/ParameterIdentifications/ParameterIdentificationEngine.cs Captures initial parameter values at start of Parameter Identification run on line 49

Sequence Diagram

sequenceDiagram
    participant Engine as ParameterIdentificationEngine
    participant PI as ParameterIdentification
    participant IP as IdentificationParameter
    participant PS as ParameterSelection
    participant Param as IParameter
    
    Note over Engine,Param: Parameter Identification Start
    Engine->>PI: StartAsync()
    Engine->>PI: CaptureIdentificationParameterInitialValues()
    PI->>IP: CaptureLinkedParameterValues()
    
    alt UseAsFactor = true
        IP->>PS: Get Parameter.Value
        PS->>Param: Value (e.g., 50)
        Param-->>PS: 50
        PS-->>IP: 50
        IP->>PS: Set InitialValue = 50
    else UseAsFactor = false
        Note over IP: Skip capturing (not needed)
    end
    
    Note over Engine,Param: Optimization Runs...
    Note over Param: Parameter may change during optimization
    
    Note over Engine,Param: Transfer Results to Simulation
    Engine->>IP: OptimizedParameterValueFor(optimalValue=10, linkedParameter)
    
    alt UseAsFactor = true AND InitialValue exists
        IP->>PS: Get InitialValue
        PS-->>IP: 50 (captured value)
        IP-->>Engine: 10 * 50 = 500
    else UseAsFactor = true AND InitialValue is null
        Note over IP: Fallback for backward compatibility
        IP->>PS: Get Parameter.Value
        PS-->>IP: Current value
        IP-->>Engine: 10 * Current value
    else UseAsFactor = false
        IP-->>Engine: 10 (optimalValue)
    end
Loading

if (linkedParameter.InitialValue.HasValue)
return optimalValue * linkedParameter.InitialValue.Value;

// Fall back to the old method if the PI was last run before this feature
Copy link
Member Author

Choose a reason for hiding this comment

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

I included this for projects where the PI has not run since the feature was introduced. It falls back to the old behavior.

@rwmcintosh rwmcintosh merged commit e00e28b into develop Jan 27, 2026
12 checks passed
@rwmcintosh rwmcintosh deleted the 2754-parameter-identification-erroneous-behavior-of-transfer-results-to-simulation-in-combination-with-use-as-factor branch January 27, 2026 19:29
@github-project-automation github-project-automation bot moved this to Done in V12.3 Jan 27, 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.

Parameter Identification: erroneous behavior of "Transfer results to Simulation" in combination with "Use as factor"

2 participants