-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add diagnostics to Gibbs reactor tests #1305
Add diagnostics to Gibbs reactor tests #1305
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1305 +/- ##
==========================================
- Coverage 77.58% 77.58% -0.01%
==========================================
Files 391 391
Lines 64288 64288
Branches 11815 11815
==========================================
- Hits 49881 49878 -3
- Misses 11825 11829 +4
+ Partials 2582 2581 -1 ☔ View full report in Codecov by Sentry. |
Co-authored-by: MarcusHolly <[email protected]>
…reactor_diagnostics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, I spotted a couple typos and have one question about the solution checks.
assert pytest.approx(250.06, abs=1e-1) == value( | ||
methane.fs.unit.outlet.flow_mol[0] | ||
) | ||
assert pytest.approx(0.0, abs=1e-4) == value( | ||
methane.fs.unit.outlet.mole_frac_comp[0, "CH4"] | ||
) | ||
assert pytest.approx(0.103863, abs=1e-4) == value( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did these values change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added scaling to satisfy the numerical diagnostics tests. I don;t recall the values changed that significantly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Had the same question about the change in values but that has been asnwered
Co-authored-by: Brandon Paul <[email protected]>
Requires #1284
Summary/Motivation:
Adds tests to assert no structural or numerical issues are present in Gibbs reactor tests.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: