idaes-pse
idaes-pse copied to clipboard
Add parallel constraint/variable check to `report_numerical_issues`
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:
- I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
- 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.
Not sure why all Andrew's commits from #1375 are showing up here...
Looks like this is breaking tests with new numerical warnings. I'll have to look into this.
@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.
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 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).
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.
If we want to use a (in my opinion) better method for calculating parallel constraints, #1395 needs to be merged before this.