Remove duplication of string identifiers in Phase#724
Closed
ischoegl wants to merge 3 commits intoCantera:masterfrom
Closed
Remove duplication of string identifiers in Phase#724ischoegl wants to merge 3 commits intoCantera:masterfrom
Phase#724ischoegl wants to merge 3 commits intoCantera:masterfrom
Conversation
7c56586 to
ff15cdf
Compare
Codecov Report
@@ Coverage Diff @@
## master #724 +/- ##
==========================================
+ Coverage 70.69% 70.69% +<.01%
==========================================
Files 372 372
Lines 43605 43606 +1
==========================================
+ Hits 30825 30827 +2
+ Misses 12780 12779 -1
Continue to review full report at Codecov.
|
ff15cdf to
ea65ba6
Compare
* reference phases using Phase::id instead * create private vcs_VolPhase::m_phaseID * vcs_VolPhase::PhaseID() replaces vcs_VolPhase::PhaseName * clarify conflation of 'phase id' in vcs_solve (which references a numeric phase number) rather than a string 'phase id' as used elsewhere * i.e. VCS_SOLVE::m_phaseNum replaces VCS_SOLVE::m_phaseID
7daad1f to
3c98504
Compare
Phase
319c204 to
5bf6357
Compare
This was referenced Oct 8, 2019
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
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.
Deprecate
Phase::namein favor ofPhase::idCurrently, there is a duplication of
Phase::name()andPhase::id(), where the former is supposed to be unique, whereas the latter is consistent with the phase id specified in the input file. In current Cantera, however, the context is always clear, so the differentiation is not needed.A differentiation of
nameandphase_iddoes make sense forSolutionobjects in the Python interface (and potentially serialization ofSolutionin C++). To preserve this capability, thenameis reassigned to the C++ objectSolution(see #696).Further:
Currently, phase name/id can be used when accessing species within a
Phase, i.e.The latter are, however, not used. I.e. per @speth's comment in #696
Changes proposed in this pull request:
Phase::namebyPhase::idin equilibrium calculations and thermo phasesSpeciesIndexin Cantera 2.5Pending
At this point, the only instance where
Phase::nameis still required is the Python interface (several tests checknamerather thanID). Once #696 is merged,namewill be part of the C++Solutionobject, i.e.Phase::name()can be deprecated.