idaes-pse icon indicating copy to clipboard operation
idaes-pse copied to clipboard

Add parallel constraint/variable check to `report_numerical_issues`

Open Robbybp opened this issue 11 months ago • 7 comments

Fixes #1380

Summary/Motivation:

These checks are fast and precise, so they should go in the general high-level reporting method.

Changes proposed in this PR:

  • Add parallel constraint/variable checks to numerical warnings section of report_numerical_issues
  • Update expected strings in tests

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Robbybp avatar Mar 23 '24 21:03 Robbybp

Not sure why all Andrew's commits from #1375 are showing up here...

Robbybp avatar Mar 23 '24 21:03 Robbybp

Looks like this is breaking tests with new numerical warnings. I'll have to look into this.

Robbybp avatar Mar 24 '24 19:03 Robbybp

@Robbybp Taking a quick look at the CSTR test, it is because the toolbox is now detecting near parallel components (outlet temperature and reaction rate in the case of the CSTR). I suspect some of these would fall under the category of "expected" (given some of the test models are very simple).

I did a quick check of setting the default tolerance for near-parallel components to 1e-6 which brought the number of failures down to around 14.

andrewlee94 avatar Mar 25 '24 14:03 andrewlee94

I did a quick check of setting the default tolerance for near-parallel components to 1e-6 which brought the number of failures down to around 14.

Tightening the tolerance to 1e-8 seems to help even more. This is backwards-incompatible, but I'm fine with tightening the tolerance here, as I think 1e-8 as a "distance from singularity" tolerance is more consistent with our other tolerances for large/small coefficients and residuals.

Robbybp avatar Apr 10 '24 13:04 Robbybp

@Robbybp That sounds fine to me too - I am not sure where the value of 1e-4 came from anyway (it was probably arbitrary). I don't think it is a huge issue for backward compatibility either, but we will need to update the relevant test cases to be "more parallel" (which should just mean changing the coefficient in the duplicate constraint).

andrewlee94 avatar Apr 10 '24 15:04 andrewlee94

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.62%. Comparing base (f8ccdec) to head (c34099b). Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1385      +/-   ##
==========================================
- Coverage   77.63%   77.62%   -0.01%     
==========================================
  Files         391      391              
  Lines       64375    64387      +12     
  Branches    14257    14260       +3     
==========================================
+ Hits        49978    49982       +4     
- Misses      11827    11834       +7     
- Partials     2570     2571       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 10 '24 17:04 codecov[bot]

If we want to use a (in my opinion) better method for calculating parallel constraints, #1395 needs to be merged before this.

dallan-keylogic avatar Apr 10 '24 21:04 dallan-keylogic