Skip to content

Comments

Fixes #2731 Follow up on CoPilot suggestions#2749

Merged
rwmcintosh merged 5 commits intodevelopfrom
2731-follow-up-on-copilot-suggestions-from-develop---main-merge
Jan 23, 2026
Merged

Fixes #2731 Follow up on CoPilot suggestions#2749
rwmcintosh merged 5 commits intodevelopfrom
2731-follow-up-on-copilot-suggestions-from-develop---main-merge

Conversation

@rwmcintosh
Copy link
Member

Fixes #2731 Follow up on CoPilot suggestions

Description

During merge from develop to main CoPilot had some suggestions but we did not want to rebuild the release binaries for these suggestions so we delayed until 12.3

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

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

Copilot AI review requested due to automatic review settings January 22, 2026 19:21
@rwmcintosh rwmcintosh requested a review from Yuri05 January 22, 2026 19:21
@rwmcintosh rwmcintosh self-assigned this Jan 22, 2026
@rwmcintosh rwmcintosh added the dev-only Assign to issues that do not need to be included in any release notes label Jan 22, 2026
@rwmcintosh rwmcintosh added this to V12.3 Jan 22, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request implements code quality improvements suggested by CoPilot that were delayed from release 12.2 to 12.3. The changes include import statement reorganization, removal of unused code, whitespace cleanup, and one float comparison modification.

Changes:

  • Reorganized import statements to follow C# conventions (System namespaces first, alphabetically ordered)
  • Removed unused BouncyCastle import and cleaned up trailing whitespace
  • Modified float zero comparison pattern from == 0 to 0.0f.Equals()
  • Removed redundant variable assignment in Ruby build script

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/OSPSuite.Infrastructure.Import/Core/DataSource.cs Reordered imports to System namespaces first, removed unused BouncyCastle import, cleaned up whitespace
src/OSPSuite.Core/Domain/Services/SensitivityAnalyses/SensitivityAnalysisRunResultCalculator.cs Changed float zero comparison from == 0 to 0.0f.Equals()
rakefile.rb Removed redundant intermediate variable str = in find_token method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 22, 2026

Greptile Summary

This PR addresses CoPilot suggestions from the develop-to-main merge by improving code quality across multiple files. The main changes include:

  • Method rename for clarity: IsValid()IsFinite() across FloatExtensions and DoubleExtensions to better reflect that the method checks for finite values (not NaN or Infinity)
  • Code cleanup: Removed redundant variable assignment in rakefile.rb, removed unused import (static Org.BouncyCastle.Bcpg.Attr.ImageAttrib), reordered using statements, and cleaned up trailing whitespace
  • Zero comparison update: Changed float zero comparison from the non-idiomatic 0.0f.Equals(defaultPKValue) to use Math.Abs(defaultPKValue) < float.Epsilon

Critical Issue Found: The zero comparison logic in SensitivityAnalysisRunResultCalculator.cs:99 has a bug. Math.Abs(defaultPKValue) < float.Epsilon will not catch exact zero values because 0 < float.Epsilon evaluates to false. The original defaultPKValue == 0 comparison was correct and should be restored.

Confidence Score: 3/5

  • Not safe to merge - contains a logic error that breaks zero detection
  • While most changes are good code quality improvements (method renaming, cleanup), there's a critical logic error in the zero comparison that will cause defaultPKValue == 0 cases to not be detected, breaking sensitivity analysis calculations for zero PK parameter values
  • src/OSPSuite.Core/Domain/Services/SensitivityAnalyses/SensitivityAnalysisRunResultCalculator.cs requires immediate attention for the zero comparison bug

Important Files Changed

Filename Overview
src/OSPSuite.Core/Extensions/FloatExtensions.cs Renamed IsValid() to IsFinite() and moved to end of file with XML documentation
src/OSPSuite.Core/Extensions/DoubleExtensions.cs Renamed IsValid() to IsFinite() for consistency with float extensions
src/OSPSuite.Core/Domain/Services/SensitivityAnalyses/SensitivityAnalysisRunResultCalculator.cs Changed zero comparison from 0.0f.Equals() to Math.Abs() < float.Epsilon, updated NaN check to IsFinite()

Sequence Diagram

sequenceDiagram
    participant Client as Client Code
    participant QVU as QuantityValuesUpdater
    participant RC as ResidualCalculator
    participant SARC as SensitivityAnalysisRunResultCalculator
    participant FE as FloatExtensions
    participant DE as DoubleExtensions

    Note over FE,DE: Renamed IsValid() → IsFinite()

    Client->>QVU: Update parameter value
    QVU->>DE: parameterValue.Value.IsFinite()
    DE-->>QVU: Returns true if not NaN/Infinity
    
    Client->>RC: Calculate residuals
    RC->>FE: simulatedValue.IsFinite()
    FE-->>RC: Returns true if not NaN/Infinity
    RC->>FE: observedValue.IsFinite()
    FE-->>RC: Returns true if not NaN/Infinity
    
    Client->>SARC: Calculate PK sensitivity
    SARC->>FE: defaultPKValue.IsFinite()
    FE-->>SARC: Returns true if not NaN/Infinity
    SARC->>SARC: Math.Abs(defaultPKValue) < float.Epsilon
    Note over SARC: Zero check logic issue:<br/>Won't catch exact 0 values
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copilot AI review requested due to automatic review settings January 23, 2026 14:56
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings January 23, 2026 15:07
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

8 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@rwmcintosh rwmcintosh requested a review from Yuri05 January 23, 2026 15:27
@rwmcintosh rwmcintosh merged commit 1699e95 into develop Jan 23, 2026
12 checks passed
@rwmcintosh rwmcintosh deleted the 2731-follow-up-on-copilot-suggestions-from-develop---main-merge branch January 23, 2026 16:14
@github-project-automation github-project-automation bot moved this to Done in V12.3 Jan 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev-only Assign to issues that do not need to be included in any release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Follow up on CoPilot suggestions from develop -> main merge

2 participants