Deprecate unused, untested, and obsolete code#1455
Merged
ischoegl merged 15 commits intoCantera:mainfrom Mar 14, 2023
Merged
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 |
ischoegl
requested changes
Mar 11, 2023
Member
ischoegl
left a comment
ischoegl
left a comment
There was a problem hiding this comment.
Thanks for going over the API … I just have a couple of minor comments.
ischoegl
reviewed
Mar 12, 2023
5 tasks
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'
ischoegl
approved these changes
Mar 14, 2023
Member
ischoegl
left a comment
ischoegl
left a comment
There was a problem hiding this comment.
Thank you, this all looks good to me!
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
IonsFromNeutralVPSSTPand 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, andTransportclassesPDSSand its derivativesDYLD_LIBRARY_PATHnot 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