Preparations for removal of CTI/XML#1159
Conversation
e900176 to
0bd0ded
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
bryanwweber
left a comment
There was a problem hiding this comment.
Thanks for taking this on @speth! Just a few small comments here.
| 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" |
There was a problem hiding this comment.
I think switching this to DeprecationWarning would be better.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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_formatwarningsSince the message is longer though, it doesn't bother me too much.
There was a problem hiding this comment.
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
mainmethod), 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
printanyway - The entire script is deprecated and I'm planning deleting it as soon as we release Cantera 2.6.
ischoegl
left a comment
There was a problem hiding this comment.
The @allow_deprecated decorator is quite nice!
Beyond, I have a couple of comments.
| 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."); |
There was a problem hiding this comment.
Should these be regular single quotes rather than back-ticks? (Here and elsewhere)
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.
| 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" |
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.
elements.xmlck2yaml,cti2yaml, andctml2yamlinstead of CTI or XML files these tests can be maintained even after the removal of the CTI and XML functionalitycti2yamlto create an empty Kinetics model when one is implied (for example, by theideal_gasdirective)T-maxfor species thermo in XML to YAML conversionsctml_writertoSIThermoPhase::getParameters(int, double*)andThermoPhase::setParameters(int, double*)Checklist
scons build&scons test) and unit tests address code coverage