-
-
Notifications
You must be signed in to change notification settings - Fork 397
VCS equilibrium solver cleanup #2061
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2061 +/- ##
==========================================
+ Coverage 76.75% 77.21% +0.45%
==========================================
Files 457 456 -1
Lines 53742 53072 -670
Branches 9122 8999 -123
==========================================
- Hits 41250 40979 -271
+ Misses 9384 9046 -338
+ Partials 3108 3047 -61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ischoegl
left a comment
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.
Thanks, @speth, for preparing to tackle some long-standing issues. Given the late review, I'll approve as is, as I'm also under the assumption that this will have additional rounds of follow-up (apart from my comment, I'm also not a big fan of plogf logging, but all of this is definitely out of scope).
The CI coverage of 68.4% could be higher, but then there's always the issue that some of the corner cases are hard to catch.
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.
Great to see most of those defines gone, but I'm wondering whether we'd like to keep this at all? Shouldn't we at least use some enum's? At the same time, it's ok to leave this for a later phase in the cleanup.
|
Yes, this is certainly not the end of what's possible for cleanup of the VCS solver, but I thought this was a good start. The low patch coverage is driven in large part by the verbose logging options, most of which are not exercised in the test suite since minor numerical differences could lead to large differences in the exact solver path. I'd look more at the increase in overall project coverage (+0.45%) as a win. |
Changes proposed in this pull request
This PR includes some broad-ranging refactoring and cleanup of the "VCS" equilibrium solver. I've done this with the idea that the only real "public" interface is through the
Mixtureclass, so there are numerous API changes within thevcs_*classes without any attempt to keep backward compatibility. Some specific changesvcs_SpeciesPropertiesandVCS_SPECIES_THERMOgotostatementsMixtureinterfaceThere are no changes to the results from the solver, other than some minor changes in the high logging level outputs. This was done with the hopes of making it a little bit easier to investigate some of the long-standing bugs in the VCS equilibrium solver, but I didn't run across any smoking guns while putting this together, so all of those issues are still unresolved.
AI Statement (required)
Significant portions of code or documentation were generated with AI (ChatGPT Codex using the GPT-5.1-Codex model), including logic and implementation decisions. All generated code and documentation were reviewed and understood by the contributor.
Checklist
scons build&scons test) and unit tests address code coverage