Transitional handling of 'gri30_mix' and 'gri30_multi' phases in YAML #737
Conversation
ischoegl
force-pushed
the
deprecate-gri30-phases
branch
2 times, most recently
from
365508b to
dcad9ab
Compare
Codecov Report
@@ Coverage Diff @@
## master #737 +/- ##
==========================================
+ Coverage 71.4% 71.41% +<.01%
==========================================
Files 372 372
Lines 43859 43878 +19
==========================================
+ Hits 31317 31334 +17
- Misses 12542 12544 +2
Continue to review full report at Codecov.
|
ischoegl
force-pushed
the
deprecate-gri30-phases
branch
from
85dcc83 to
e2224b3
Compare
There was a problem hiding this comment.
I think this solution is not surgical enough at the moment. The main problem I foresee is users who deliberately create their own version of GRI-3.0 with these phase definitions in them. I'm always surprised when I see someone on the Users' Group who has a copy of gri30.cti in their working directory, but as I recall it has happened multiple times. Is there any way to have this warning raised if and only if the gri30.yaml file comes from the installed data location?
ischoegl
force-pushed
the
deprecate-gri30-phases
branch
3 times, most recently
from
39fe258 to
12d441b
Compare
|
@bryanwweber ... thank you for the review! Regarding your comment about users potentially keeping local copies of However, you are correct that the solution needs to be more surgical: a custom
|
ischoegl
force-pushed
the
deprecate-gri30-phases
branch
2 times, most recently
from
88bcdd2 to
a44d00d
Compare
|
@bryanwweber ... I believe it's now all fixed (and It's hard to make it more specific as the warning is raised from the C++ layer (i.e. I cannot refer to MATLAB specific syntax). As an aside, the |
There was a problem hiding this comment.
For the Python interface, at least, it doesn't appear that the appropriate transport model is set (I could be wrong, I'm not sure if there's an extra step that calls into C++ that I'm missing in the diff view here). If a user is passing 'gri30_multi' they would expect that Multi-component transport is enabled, but they'll get an object with mixture-averaged transport. Can that be resolved?
MATLAB can't be tested on any of the public, free CI services because we can't install MATLAB on them. @speth has a buildbot instance set up on some hardware at MIT that runs the (anemic) MATLAB test suite IIRC, but I can't recall the URL for that, or if it's public facing. |
I cannot reproduce this: |
ischoegl
force-pushed
the
deprecate-gri30-phases
branch
from
a44d00d to
4acb3dc
Compare
|
Regarding MATLAB: and, in the same session (warning is only shown the first time) |
ischoegl
force-pushed
the
deprecate-gri30-phases
branch
from
4acb3dc to
980e3a4
Compare
Thanks for checking! That's what I hoped would happen but couldn't tell from the diff view 😄 |
You're welcome. For these exceptions, the transport model simply goes by phase name (which doesn't change), and the conversion is caught elsewhere. (Transport models get instantiated much later.) I actually added two lines to the unit tests to make sure that this behavior won't change ... |
|
@ischoegl What about this warning message? if name in ['gri30_mix', 'gri30_multi']:
input_file = pystr(root[stringify("__file__")].asString())
# catch transitional behavior
warnings.warn(dedent("""
The phase named '{}' is not defined in the input file '{}'.
Examples that use this phase name are deprecated. Instead, you
should use one of the following options to choose a transport
model:
* ct.Solution("gri30.yaml") # Mixture-averaged transport, default
* ct.Solution("gri30.yaml", transport_model="mix") # Mixture-averaged transport
* ct.Solution("gri30.yaml", transport_model=None) # No transport model, useful for 0-D simulations
* ct.Solution("gri30.yaml", transport_model="multicomponent") # Multicomponent transport
""".format(name, input_file)), DeprecationWarning)Maybe add Rewriting this message, I went back and forth on whether user-generated input files should be handled in this way. On the one hand, it is nice for a user if they're following an example and the end up using one of these phase names and get warned about it. On the other hand, if I were that user, I'm probably very new to Cantera, and having warnings like this thrown at me is not super helpful. FYI, One edge case I just thought of (this is a little convoluted, but bear with me 😄 ):
In [3]: ct.Solution("./gri30.yaml", "gri30_mix")
<ipython-input-3-1e59bb5fc60e>:1: DeprecationWarning:
The phase named 'gri30_mix' is not defined in the input file './gri30.yaml'.
Examples that use these phases are deprecated.
Instead, you should use one of the following options:
* ct.Solution('gri30.yaml') # Mixture-averaged transport
* ct.Solution('gri30.yaml', transport_model='mix') # Mixture-averaged transport
* ct.Solution('gri30.yaml', transport_model=None) # No transport model, useful for 0-D simulations
* ct.Solution('gri30.yaml', transport_model='multicomponent') # Multicomponent transport
ct.Solution("./gri30.yaml", "gri30_mix")
---------------------------------------------------------------------------
CanteraError Traceback (most recent call last)
~/GitHub/cantera/interfaces/cython/cantera/base.pyx in cantera._cantera._SolutionBase._init_yaml()
113 try:
--> 114 phaseNode = root[stringify("phases")].getMapWhere(
115 stringify("name"), stringify(name))
CanteraError:
***********************************************************************
InputFileError thrown by AnyValue::getMapWhere:
Error on line 16 of ./gri30.yaml:
List does not contain a map where 'name' = 'gri30_mix'
| Line |
| 11 | date: Fri, 13 Dec 2019 16:19:11 -0500
| 12 |
| 13 | units: {length: cm, time: s, quantity: mol, activation-energy: cal/mol}
| 14 |
| 15 | phases:
> 16 > - name: gas
^
| 17 | thermo: ideal-gas
| 18 | elements: [O, H, C, N, Ar]
| 19 | species: [H2, H, O, O2, OH, H2O, HO2, H2O2, C, CH, CH2, CH2(S), CH3, CH4,
***********************************************************************
During handling of the above exception, another exception occurred:
CanteraError Traceback (most recent call last)
<ipython-input-3-1e59bb5fc60e> in <module>
----> 1 ct.Solution("./gri30.yaml", "gri30_mix")
~/GitHub/cantera/interfaces/cython/cantera/base.pyx in cantera._cantera._SolutionBase.__cinit__()
54 # Parse inputs
55 if infile.endswith('.yml') or infile.endswith('.yaml') or yaml:
---> 56 self._init_yaml(infile, name, adjacent, yaml)
57 elif infile or source:
58 self._init_cti_xml(infile, name, adjacent, source)
~/GitHub/cantera/interfaces/cython/cantera/base.pyx in cantera._cantera._SolutionBase._init_yaml()
128 * ct.Solution("gri30.yaml", transport_model=None) # No transport model, useful for 0-D simulations
129 * ct.Solution("gri30.yaml", transport_model="multicomponent") # Multicomponent transport
--> 130 """.format(name, input_file)), DeprecationWarning)
131 phaseNode = root[stringify("phases")].getMapWhere(
132 stringify("name"), stringify("gri30"))
CanteraError:
***********************************************************************
InputFileError thrown by AnyValue::getMapWhere:
Error on line 16 of ./gri30.yaml:
List does not contain a map where 'name' = 'gri30'
| Line |
| 11 | date: Fri, 13 Dec 2019 16:19:11 -0500
| 12 |
| 13 | units: {length: cm, time: s, quantity: mol, activation-energy: cal/mol}
| 14 |
| 15 | phases:
> 16 > - name: gas
^
| 17 | thermo: ideal-gas
| 18 | elements: [O, H, C, N, Ar]
| 19 | species: [H2, H, O, O2, OH, H2O, HO2, H2O2, C, CH, CH2, CH2(S), CH3, CH4,
***********************************************************************Anyways, this sequence of events should raise an error (because the name they passed really isn't defined), so I'm not sure why the user would expect it to work in the first place. And it seems pretty unlikely to happen. Just thought I'd throw it out there. |
|
@bryanwweber ... thanks for the suggestion. Will adopt. Otherwise, interesting scenario! Unfortunately, this is even raised if the phase name is not changed in |
|
If we are effectively making the data files part of the API, where changes are subject to deprecation warnings, then it might be worth discussing a more general approach to making such changes, rather than a one-off for just the One possible implementation would be to add a field to either the file or the phase definition that is checked for when the file is loaded, and used to issue a deprecation warning. This would have the benefit of not affecting copies of files (modified or not) that users have placed on their system outside the Cantera data directories. For the YAML format, a file-level deprecation could be written: generator: ctml2yaml
cantera-version: 2.5.0a3
date: Thu, 21 Nov 2019 19:34:25 -0500
input-files: [build\data\gri30.xml]
deprecated: >
The input file silicon_carbide.yaml is deprecated and will be removed
after Cantera X.YOr an individual phase definition could be deprecated by adding: phases:
- name: gri30_mix
thermo: ideal-gas
transport: mixture-averaged
kinetics: gas
reactions: all
state: {T: 300.0 K, P: 1.01325e+05 Pa}
deprecated: >
The gri30_mix phase definition will be removed from gri30.yaml
after Cantera X.Y.In CTI, there could be a top-level ideal_gas(name = "gri30_mix",
elements = " O H C N Ar ",
species = """ H2 H O O2 OH H2O HO2 H2O2 C CH
CH2 CH2(S) CH3 CH4 CO CO2 HCO CH2O CH2OH CH3O
CH3OH C2H C2H2 C2H3 C2H4 C2H5 C2H6 HCCO CH2CO HCCOH
N NH NH2 NH3 NNH NO NO2 N2O HNO CN
HCN H2CN HCNN HCNO HOCN HNCO NCO N2 AR C3H7
C3H8 CH2CHO CH3CHO """,
reactions = "all",
transport = "Mix",
options = ["deprecated: The gri30_mix phase definition will be removed from gri30.cti"
" after Cantera X.Y."]
initial_state = state(temperature = 300.0,
pressure = OneAtm) ) |
|
@speth ... I think adding deprecation tags is a good idea. The question boils down to: Would you suggest to ditch the The one issue that would remain is that the default phase for |
I'm not sure what you mean by auto-generated, but it's perfectly fine to have a YAML file without transport data.
I don't think that would be necessary. If the default changes from no transport to mixture-averaged, I don't see a problem with that, or a need to warn anyone. IMO the one advantage this approach has is showing a way to turn off transport and reduce the time it takes to load the file, especially for large mechanisms where the only real possibility is zero-D simulations anyways. But we could add that to an example |
|
@bryanwweber ... to clarify CTI/XML offer the following phases The (current) gri30.yaml generated by Note that any As indicated, the question boils down to whether:
If the consensus is to move away from (2), I'd suggest closing this PR and starting a new one as none of this PR's code carries over. |
There was a problem hiding this comment.
The one issue that would remain is that the default phase for
gri30.yamldoes not have a transport model, and thus is not compatible with default behavior of auto-generated yaml files, even if deprecation tags are used to eliminate other superfluous phase entries. Overall, I’d still advocate for a consistent default (e.g. mixture-averaged transport if transport data are available), which i believe does require some input-specific transitional handling as this PR proposes.
Making gri30.yaml consistent with what a user would end up generating from their own Chemkin input files was the main point of removing the custom phase definitions of gri30.cti. I agree that "mixture-averaged transport if transport data are available", which is the convention inherited from ck2cti is a reasonable default.
However, you are correct that the solution needs to be more surgical: a custom
yamlfile may contain validgri30_mixdefinitions, i.e. they should be used when present. A case in point is anything converted usingcti2yamlorctml2yaml... which applies togri30_highT.yamletc.
The logic that applies eliminating the alternate phase definitions from gri30 applies to gri30_highT as well. The only thing that makes this trickier is that gri30_highT.cti seems to be a manually-edited copy of gri30.cti, rather than generated Chemkin-format files. So the only way to implement that change will be to commit the desired version of gri30_highT.yaml directly, perhaps as part of #768.
One edge case I just thought of (this is a little convoluted, but bear with me ):
- User creates their own
gri30.yamlby converting the CK files- User changes the default phase name in the file, either during conversion or afterwards
- User tries to load that file and passes one of these deprecated phase names
- Kaboom!
This does indeed seem pretty convoluted, and I think the conditions are even more specific than this: the user would have to remove both the gri30 and (say) the gri30_mix phase definitions, but then try to load the gri30_mix phase definition. And the only thing wrong with the error message is that it specifies that the missing name is gri30 rather than gri30_mix.
|
@speth Are you no longer in favor of the phases:
- name: example
thermo: something-deprecated
deprecated: The 'something-deprecated' phase thermo type is deprecated...
...Obviously, this would have to be added manually to any files that used that thermo type, and presumably there would be deprecation warnings when the thermo type was instantiated, but it would still be a useful field to have IMO. If you would still like to pursue that avenue, I agree with @ischoegl that this PR should be closed. My preference is for suggestion (1) from @ischoegl, that is, retain the
I think the more likely case is a user converts the GRI-3.0 CK files to YAML, resulting in the default name of |
That was my understanding before starting this PR, which led me to implementation (2) 😉. I personally agree with @speth that it's preferable to have consistent behavior. The switch of formats is imho an opportunity to get rid of inconsistent implementations. I am not opposed to the PS: the convoluted error message can be fixed/avoided with fairly little effort. |
No, I still think it would be useful, and I'd like to see it before we act on #768. Either the addition of a Perhaps we should put together an implementation of the |
ischoegl
force-pushed
the
deprecate-gri30-phases
branch
from
980e3a4 to
de6ba7d
Compare
|
@speth and @bryanwweber ... I finished implementation (2), and made sure that the awkward error Bryan pointed out will not appear (there is a unit test for it).
To reiterate: if the above is the objective (as had been my understanding), then there are two implementation paths for how users would upgrade existing custom models: (1) the
(2) this PR:
Imho the main advantage of (2) is that there will be exactly one version of In summary, while I believe the |
|
PS: the two version of the warnings Python C++/Matlab version (uses a temporarily modified example for illustration purposes) |
I don't necessarily think this is true. I don't see a problem with changing the default phase in
This is true, but I don't think it's that big of an advantage. If we are going to be removing/changing other input files after using the Another thought... From the warning message: I know that I wrote this message, but I keep thinking of ways it could be misinterpreted. For this line, this is only definitely the default for the built-in There are just so many edge cases with trying to guess what a user meant to do with their own input files, I just can't shake the feeling that this warning should be limited only to files distributed with Cantera. 😞 Sorry to put this PR through the ringer, @ischoegl thanks for the constructive discussion! |
|
@bryanwweber ... I’m open for suggestions/clarifications. Getting the warning message right now should pay off later. |
|
Another comment: rather than porting CTI to YAML and immediately adding a |
|
Closing as solution proposed in #768 is less intrusive and able to handle other cases as well. |
Please fill in the issue number this pull request is fixing:
Fixes #736
Changes proposed in this pull request:
gri30_mixandgri30_multiphase definitions for YAML and issue warningsgri30.ctiandgri30.xmlBygri30.yamlin test suiteAn attempt to load deprecated phases from
gri30.yamlhas the expected behavior, but issues a warning: