Skip to content
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

Improve guidance for transition of deprecated thermo models #1008

Open
ischoegl opened this issue Apr 7, 2021 · 5 comments
Open

Improve guidance for transition of deprecated thermo models #1008

ischoegl opened this issue Apr 7, 2021 · 5 comments
Labels
Input Input parsing and conversion (for example, ck2yaml)

Comments

@ischoegl
Copy link
Member

ischoegl commented Apr 7, 2021

Problem description

As illustrated in a recent UG post, the deprecation of some thermo models in 2.5 (e.g. incompressible_solid) does not provide sufficient feedback on details how those old models should be replaced, i.e.

DeprecationWarning: class ConstDensityThermo: To be removed after Cantera 2.5.0. Consider replacing with LatticePhase or IdealSolidSolnPhase

Beyond, cti2yaml does not appear to recognize the thermo model either, and simply states

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/cantera/cti2yaml.py", line 1567, in convert
    exec(code)
  File "Mayur.cti", line 81, in <module>
    incompressible_solid(
NameError: name 'incompressible_solid' is not defined

(note: this requires the development version due to #996)

Steps to reproduce

see UG post

Possible Solutions

As the phases are already marked for deprecation (and are already removed from the development version), there isn't much that can be done for 2.6. The main alternative would be to provide some guidance for transitions from CTI to YAML on the website.

The issue with cti2yaml not recognizing incompressible_solid should be addressed, however. One solution would be to raise an error, and explicitly direct the user to the webpage that provides information on recommended steps.

Behavior

System information

Other thoughts

Importing the offending file on the development branch results in the following error:

cantera._cantera.CanteraError: 
***********************************************************************
CanteraError thrown by call_ctml_writer:
Error converting input file "./Mayur.cti" to CTML.
Python command was: '/usr/bin/python3'
The exit code was: 4
-------------- start of converter log --------------
/usr/local/lib/python3.6/dist-packages/cantera/ctml_writer.py:2729: ResourceWarning: unclosed file <_io.TextIOWrapper name='./Mayur.cti' mode='r' encoding='ANSI_X3.4-1968'>
  text = open(filename, open_mode).readlines()
NameError on line 81 of ./Mayur.cti:
name 'incompressible_solid' is not defined

| Line |
|   76 | 	initial_state = state( mole_fractions = "electron:1.0")
|   77 | )
|   78 | 
|   79 | # SEI - Lithium ethylene dicarbonate LEDC - (CH2OCO2Li)2
|   80 | #------------------------------------------------------------------
>   81 > incompressible_solid(
|   82 | 	name = "LEDC",
|   83 | 	elements = "C H O Li",
|   84 | 	species = "LEDC",
--------------- end of converter log ---------------
***********************************************************************

which could be improved also (e.g. fix the ResoureWarning, and provide clues for the user on how to proceed)

@ischoegl ischoegl added the Input Input parsing and conversion (for example, ck2yaml) label Apr 7, 2021
@bryanwweber
Copy link
Member

This also affects one of the Jupyter examples, the 1-D PFR example that uses this input file: https://github.com/Cantera/cantera-jupyter/blob/main/reactors/data/SiF4_NH3_mec.cti

@speth
Copy link
Member

speth commented Apr 8, 2021

I think the behavior in the development version is basically what should be expected - there's no trace of classes/models that have been previously deprecated and removed. The 2.5 release can and does issue deprecation warnings, although I'll certainly agree that in this case, some more guidance on how to adapt input files should be provided, since it requires moving the density information from the phase to the species definition. Also, I'm not quite sure where to put this on the website.

For CTI, the input modifications are a little bit annoying, given the way density information gets embedded in the species entry and the fact that the density can only be expressed as a molar volume. The phase definition should be changed as follows:

incompressible_solid(
	name = "LEDC",
	elements = "C H O Li",
	species = "LEDC",
	density = (1300, 'kg/m3'),
	initial_state = state( pressure = OneAtm, mole_fractions = "LEDC:1.0")
)

becomes:

IdealSolidSolution(
	name = "LEDC",
	elements = "C H O Li",
	species = "LEDC",
	standard_concentration = "molar_volume",
	initial_state = state( pressure = OneAtm, mole_fractions = "LEDC:1.0")
)

And the species definition should be changed to add the standardState field like this:

species(
	name = "LEDC",
	atoms = "C:4 H:4 O:6 Li:2",
	thermo = const_cp(h0 = (-1374.288, 'kJ/mol'), s0 = (88.78, 'J/mol/K')),
	standardState = constantIncompressible(molarVolume=((4*12.011+4*1.00794+6*15.9994+2*6.941)/1300))
)

For YAML input files, this is considerably simpler. The phase definition just needs to change the thermo field from constant-density to ideal-condensed (there is already a default for the standard concentration), and the corresponding species definition just requires a equation-of-state field like this:

- name: LEDC
  composition: {C: 4, H: 4, O: 6, Li: 2}
  thermo:
    model: constant-cp
    h0: -1374.288 kJ/mol
    s0: 88.78 J/mol/K
  equation-of-state:
    model: constant-volume
    density: 1300 kg/m^3

I haven't seen that ResourceWarning issue before, so I guess that's something else to track down separately.

To be able to run the version of cti2yaml from Cantera 2.5.1 on this input file, the issues with the transport model definition can be solved by just editing the file to remove the lines that say transport = "None". I did run into a couple of other issues with this input file (attached here because it's not that easy to find in that Users' Group thread) which I'll post about separately, but are fixed in the version attached here: Mayur_fixed.cti.txt.

@ischoegl
Copy link
Member Author

ischoegl commented Apr 8, 2021

Thank you, @speth - this is a nice description of how to fix the problem reported in the group posting that prompted this issue.

I think the behavior in the development version is basically what should be expected - there's no trace of classes/models that have been previously deprecated and removed. The 2.5 release can and does issue deprecation warnings, although I'll certainly agree that in this case, some more guidance on how to adapt input files should be provided, since it requires moving the density information from the phase to the species definition. Also, I'm not quite sure where to put this on the website.

I do agree that the current behavior is not unexpected and perfectly fine from a development point of view. However, I am more concerned about future questions about why cantera doesn't recognize a model in an input file that appears to be perfectly fine, and has an associated publication (as in the mayur case failing with a relatively unhelpful message name 'incompressible_solid' is not defined). It would be relatively straightforward to catch deprecated model names, redirect users to a web page that lists obsolete model definitions along with recommendations, and thus avoid long threads on the user group. (Same goes for cti2yaml and ctml2yaml, btw.) Apart from thermo models, there are also a couple of reaction type definitions that are no longer supported.

@speth
Copy link
Member

speth commented Apr 8, 2021

It would be relatively straightforward to catch deprecated model names

I think you're right for cases where they can be caught in the C++ layer, where at it's simplest this would just require keeping a list of obsolete model names in the corresponding factory class. But in the case of cti2yaml, I think you have to define a method with the obsolete name for each obsolete class in order for it to raise an appropriate error, which is a little bit messier. Or just never remove anything from cti2yaml and let those errors be raised by the C++ interface at import time.

@ischoegl
Copy link
Member Author

ischoegl commented Apr 8, 2021

... where at it's simplest this would just require keeping a list of obsolete model names in the corresponding factory class.

👍 Really like the idea of handling this via factory classes for the C++ case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Input Input parsing and conversion (for example, ck2yaml)
Projects
No open projects
Development

No branches or pull requests

3 participants