Preparations for removal of CTI/XML #1159

speth · 2021-12-19T05:07:53Z

Changes proposed in this pull request

This PR takes care of changes that are needed to allow clean removal of the CTI and XML formats after the release of Cantera 2.6.

Use of CTI/XML for input/output generates deprecation warnings
CK to CTI and CTI to XML conversions generate deprecation warnings
Read element standard entropies from a YAML database, eliminating the dependence on elements.xml
Use YAML reference files for ck2yaml, cti2yaml, and ctml2yaml instead of CTI or XML files these tests can be maintained even after the removal of the CTI and XML functionality
Fix 1D solver debug mode to save residual as YAML instead of XML
Fix cti2yaml to create an empty Kinetics model when one is implied (for example, by the ideal_gas directive)
Update instructions for creating "reference" files for some 1D solver regression tests
Fix setting of T-max for species thermo in XML to YAML conversions
Update standard concentration model names when converting from XML to YAML
Fix unit handling for reactions with non-reactant orders in ctml_writer
Transition all Python tests to use YAML input files
Deprecate some additional XML helper functions and classes like toSI
Deprecate unused ThermoPhase::getParameters(int, double*) and ThermoPhase::setParameters(int, double*)
Update Python docstrings to use YAML examples instead of CTI/XML
Remove XML-centric, model-specific instructions for instantiating phases in C++
Remove test data files that are no longer used

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
- The decrease in coverage comes from a lot of XML-based initialization no longer being called, which I think is to be expected at this point.
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

codecov · 2021-12-19T16:43:38Z

Codecov Report

Merging #1159 (1c3112b) into main (76e2057) will decrease coverage by 1.19%.
The diff coverage is 41.17%.

@@            Coverage Diff             @@
##             main    #1159      +/-   ##
==========================================
- Coverage   66.30%   65.10%   -1.20%     
==========================================
  Files         315      315              
  Lines       45283    45258      -25     
  Branches    19237    19240       +3     
==========================================
- Hits        30024    29467     -557     
- Misses      12644    13329     +685     
+ Partials     2615     2462     -153

Impacted Files	Coverage Δ
include/cantera/base/global.h	`84.21% <ø> (ø)`
include/cantera/thermo/DebyeHuckel.h	`100.00% <ø> (ø)`
include/cantera/thermo/HMWSoln.h	`33.33% <ø> (ø)`
include/cantera/thermo/IdealGasPhase.h	`94.44% <ø> (ø)`
include/cantera/thermo/IdealMolalSoln.h	`100.00% <ø> (ø)`
include/cantera/thermo/LatticePhase.h	`87.50% <ø> (ø)`
include/cantera/thermo/MolalityVPSSTP.h	`80.00% <ø> (ø)`
include/cantera/thermo/StoichSubstance.h	`100.00% <ø> (ø)`
include/cantera/thermo/SurfPhase.h	`100.00% <ø> (ø)`
include/cantera/thermo/ThermoPhase.h	`30.96% <ø> (-0.68%)`	⬇️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76e2057...1c3112b. Read the comment docs.

bryanwweber

Thanks for taking this on @speth! Just a few small comments here.

data/element-standard-entropies.yaml

interfaces/cython/cantera/ctml2yaml.py

bryanwweber · 2021-12-22T22:54:24Z

interfaces/cython/cantera/ctml_writer.py

@@ -2756,6 +2761,11 @@ def convert(filename=None, outName=None, text=None):
 def main():
    if len(sys.argv) not in (2,3):
        raise ValueError('Incorrect number of command line arguments.')
+    print("WARNING: The CTI and XML input file formats are deprecated and will be\n"


I think switching this to DeprecationWarning would be better.

Using raise DeprecationWarning(...) has the effect of also printing out the stack trace, which most users are not interested in and I think has the effect of causing people to miss the actual important warning text, e.g.:

Traceback (most recent call last): File "/home/speth/src/cantera/interfaces/cython/cantera/ctml_writer.py", line 2772, in <module> main() File "/home/speth/src/cantera/interfaces/cython/cantera/ctml_writer.py", line 2764, in main raise DeprecationWarning("WARNING: The CTI and XML input file formats are deprecated and will be\n" DeprecationWarning: WARNING: The CTI and XML input file formats are deprecated and will be removed in Cantera 3.0. Use `cti2yaml.py` to convert CTI input files to the YAML format. See https://cantera.org/tutorials/legacy2yaml.html for more information.

I don't think you need to raise the DeprecatuonWarning. I think importing warnings and using that interface would be better. https://docs.python.org/3/library/warnings.html#warnings.warn

Perhaps FutureWarning would be better, too: https://docs.python.org/3/library/exceptions.html#FutureWarning

I see, I was misunderstanding how DeprecationWarning is supposed to be used, based on the fact that it is a type of Exception. But the output from warnings.warn still seems clunkier than I'd prefer:

/home/speth/src/cantera/build/python/cantera/ctml_writer.py:2765: FutureWarning: The CTI and XML input file formats are deprecated and will be removed in Cantera 3.0. Use `cti2yaml.py` to convert CTI input files to the YAML format. See https://cantera.org/tutorials/legacy2yaml.html for more information. warnings.warn(

The line reference makes sense, but I don't really get why it prints the line of code that starts the call to warnings.warn (which, if the message were shorter, would just be repeating the message again).

Looks like the recommendation in the docs is to replace warnings.formatwarning (via SO). Something like

def custom_formatwarnings(msg, category, filename, lineno, line=None): return f"{filename}:{lineno}:{msg}\n" warnings.formatwarning = custom_formatwarnings

Since the message is longer though, it doesn't bother me too much.

I don't understand what is "better" about using the warnings interface when:

This message is triggered only when this file is called as a script (i.e., through the main method), so there's no benefit/opportunity for anything to interact with settings that would either suppress this warning or elevate it into an error

All other warning output from this script is just handled by print anyway

The entire script is deprecated and I'm planning deleting it as soon as we release Cantera 2.6.

Sounds good to me!

interfaces/cython/cantera/ck2cti.py

interfaces/cython/cantera/onedim.pyx

interfaces/cython/cantera/test/test_convert.py

ischoegl

The @allow_deprecated decorator is quite nice!

Beyond, I have a couple of comments.

include/cantera/thermo/LatticePhase.h

include/cantera/thermo/SurfPhase.h

include/cantera/thermo/WaterSSTP.h

ischoegl · 2021-12-23T14:55:46Z

src/base/xml.cpp

+    warn_deprecated("XML_Node::build",
+        "\nThe CTI and XML input file formats are deprecated and will be removed in\n"
+        "Cantera 3.0. Use `cti2yaml.py` or `ctml2yaml.py` to convert CTI or XML input\n"
+        "files to the YAML format. See https://cantera.org/tutorials/legacy2yaml.html\n"
+        "for more information.");


Should these be regular single quotes rather than back-ticks? (Here and elsewhere)

On a separate note, would it make sense to promote the legacy2yaml page to a more prominent position on the tutorials page (right now it's within the input-files sub-category)?

I'm starting to wondering if a large chunk of the "Input" documentation needs to be promoted a level in the website -- Right now, it's all buried as a subsection under "tutorials", which has the effect of both putting the input file related tutorials and extra layer deep, and hiding the YAML format documentation inside the tutorials section, which isn't really correct.

Actually, I take that back - Besides being accessible from within the "Tutorials" section, the general YAML documentation is available from the main cantera.org page under Documentation -> YAML Input File Users' Guide, and the documentation for specific models is available as Documentation -> YAML Input File API Reference, which seems about right.

I'm not sure how much more prominent the legacy2yaml page needs to be. Within the tutorials section, I think it's in the right place, though I suppose it could come before the deprecated options of "creating a new CTI file" and "conversion from Chemkin to CTI". For users who don't think they need tutorials, an additional link on the "Documentation" page wouldn't hurt either.

I personally think that the input format description should be linked on the main page. It’s tangential here, and the url in the warnings is fine

interfaces/cython/cantera/test/test_purefluid.py

This is necessary so these tests can be retained after the CTI format is removed and only the cti2yaml conversion tool remains.

This is necessary so these tests can be retained after the XML format is removed and only the ctml2yaml conversion tool remains.

bryanwweber

Thanks @speth!

bryanwweber · 2022-01-05T21:46:06Z

interfaces/cython/cantera/ctml_writer.py

@@ -2756,6 +2761,11 @@ def convert(filename=None, outName=None, text=None):
 def main():
    if len(sys.argv) not in (2,3):
        raise ValueError('Incorrect number of command line arguments.')
+    print("WARNING: The CTI and XML input file formats are deprecated and will be\n"


Sounds good to me!

speth force-pushed the warn-xml branch 2 times, most recently from e900176 to 0bd0ded Compare December 19, 2021 16:19

speth marked this pull request as ready for review December 19, 2021 17:04

bryanwweber requested changes Dec 22, 2021

View reviewed changes

ischoegl requested changes Dec 23, 2021

View reviewed changes

speth added 24 commits January 4, 2022 21:57

[1D] Fix saving residual as YAML

0ced11e

[Input] Add deprecation warnings for CTI/XML conversions

a012f6c

[Input] Read element standard entropies from YAML database

aed9cbe

[Test] Use YAML input files as reference for cti2yaml tests

1f5a8a6

This is necessary so these tests can be retained after the CTI format is removed and only the cti2yaml conversion tool remains.

[Test] Sync inputs for pathological species name tests

da131c1

[Test] Use YAML input files as reference for ctml2yaml tests

1656624

This is necessary so these tests can be retained after the XML format is removed and only the ctml2yaml conversion tool remains.

[cti2yaml] Create empty kinetics model when one is implied

03767d8

[Test/1D] Update re-creation of reference profiles

b9dfdd2

[ctml2yaml] Fix typo in species max T handling

63c4286

[ctml2yaml] Use YAML-preferred name for standard concentration model

a13da14

[Input] Fix ctml_writer unit handling for non-reactant orders

0af5a3a

[Test] Use YAML input files as reference for ck2yaml tests

3620d3c

[Test] Eliminate incidental use of CTI/XML in Python thermo tests

973f9ec

[Test] Eliminate incidental use of CTI/XML in Python kinetics tests

59b3c47

[Test] Eliminate incidental use of CTI/XML in Python 1D tests

e817344

[Test] Eliminate incidental use of CTI/XML in other Python tests

8b02b64

Make CTI/XML deprecation warnings noisy

79380cb

Deprecate some XML-related functions

98db9dd

Deprecate array-based ThermoPhase::get/setParameters

8e13d2a

Use YAML in Python doc examples

1aabf59

Update Matlab toolbox for CTI/XML deprecation

27bc6c5

[Doc] Remove XML-centric, model-specific phase creation examples

4f1e7e2

Remove unused test data files

32dcea0

[Test] Eliminate some unnecessary special cases

b1718c4

[Doc] Note XML deprecation with all XML examples

1c3112b

speth force-pushed the warn-xml branch from 0bd0ded to 1c3112b Compare January 5, 2022 03:48

speth requested review from ischoegl and bryanwweber January 5, 2022 03:49

ischoegl approved these changes Jan 5, 2022

View reviewed changes

bryanwweber approved these changes Jan 5, 2022

View reviewed changes

speth merged commit 698c51c into Cantera:main Jan 5, 2022

ischoegl mentioned this pull request Feb 21, 2022

C++ Equivalent of SolutionArray Cantera/enhancements#137

Closed

speth deleted the warn-xml branch July 23, 2024 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preparations for removal of CTI/XML #1159

Preparations for removal of CTI/XML #1159

speth commented Dec 19, 2021 •

edited

Loading

codecov bot commented Dec 19, 2021 •

edited

Loading

bryanwweber left a comment

bryanwweber Dec 22, 2021

speth Dec 31, 2021

bryanwweber Dec 31, 2021

speth Jan 5, 2022

bryanwweber Jan 5, 2022 •

edited

Loading

speth Jan 5, 2022

bryanwweber Jan 5, 2022

ischoegl left a comment

ischoegl Dec 23, 2021

ischoegl Dec 23, 2021

speth Dec 31, 2021

speth Jan 5, 2022

ischoegl Jan 5, 2022 •

edited

Loading

bryanwweber left a comment

bryanwweber Jan 5, 2022

Preparations for removal of CTI/XML #1159

Preparations for removal of CTI/XML #1159

Conversation

speth commented Dec 19, 2021 • edited Loading

codecov bot commented Dec 19, 2021 • edited Loading

Codecov Report

bryanwweber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanwweber Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ischoegl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ischoegl Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

bryanwweber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speth commented Dec 19, 2021 •

edited

Loading

codecov bot commented Dec 19, 2021 •

edited

Loading

bryanwweber Jan 5, 2022 •

edited

Loading

ischoegl Jan 5, 2022 •

edited

Loading