-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
Deprecate unused, untested, and obsolete code #1455
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1455 +/- ##
==========================================
- Coverage 69.64% 69.61% -0.03%
==========================================
Files 374 373 -1
Lines 55812 55846 +34
Branches 18298 18338 +40
==========================================
+ Hits 38868 38878 +10
- Misses 14487 14514 +27
+ Partials 2457 2454 -3
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 for going over the API … I just have a couple of minor comments.
This model has had a request for tests/examples out for many years (Cantera#267) with no response, and the addition of consistency tests revealed multiple errors in the implementation (documented in Cantera#1322). Further, there is no mathematical description of the model or references to literature to use as a reference in trying to correct any of these issues. Resolves Cantera#1322.
macOS doesn't allow passing DYLD_LIBRARY_PATH through a shell, so we need to use subprocess.call to be able to call sphinx-build while including build/lib in the library path. The need to have the library path set for this is a result of the transition to linking the Python module against a shared version of libcantera.
This phase model is incomplete, has numerous thermodynamic consistency issues as documented in Cantera#1321, and has no known non-trivial examples. Resolves Cantera#1321
Deprecate 'None'
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.
Thank you, this all looks good to me!
The goal of this PR is to put a dent in the amount of unused or unusable code we're dragging around. The specific items picked out here is based partly on looking through coverage reports, and partly on various warts / oddities that I had noticed while working on other things and thought they would be better handled in a PR for this specific purpose.
Changes proposed in this pull request
IonsFromNeutralVPSSTP
and classPDSS_IonsFromNeutral
, based on its history in Improve test suite coverage or remove unused code #267 and various errors documented in IonsFromNeutralPhase has inconsistent implementations of some thermo properties #1322MaskellSolidSolnPhase
, based on errors documented in MaskellSolidSolnPhase has numerous consistency issues #1321 and a lack of meaningful examplesPhase
,ThermoPhase
,Kinetics
, andTransport
classesPDSS
and its derivativesDYLD_LIBRARY_PATH
not getting passed throughIf applicable, fill in the issue number this pull request is fixing
Checklist
scons build
&scons test
) and unit tests address code coverage