Skip to content

Detect description in cti2yaml#1205

Merged
bryanwweber merged 5 commits intoCantera:mainfrom
ischoegl:detect-description-in-conversion-scripts
Mar 3, 2022
Merged

Detect description in cti2yaml#1205
bryanwweber merged 5 commits intoCantera:mainfrom
ischoegl:detect-description-in-conversion-scripts

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Feb 21, 2022 •
edited
Loading

Changes proposed in this pull request

  • Automatically convert CTI header block to YAML description as default (?)
  • Provide option to override header conversion
  • Add 'validation', i.e. make sure that the resulting mechanism imports without errors (only checked when called from CLI - I.e. same approach as ck2yaml)

Further:

  • Add some missing description blocks that were lost during conversion
  • Move lone input file in data/sample-data to sample/data

If applicable, fill in the issue number this pull request is fixing

Partially addresses Cantera/enhancements#22 and Cantera/enhancements#23

If applicable, provide an example illustrating new features this pull request is introducing

$ cti2yaml haca2.cti 
Wrote YAML mechanism file to 'haca2.yaml'.
Mechanism contains 3 species and 1 reactions.
Validating mechanism...
PASSED

(after also checking interface phases, this actually fails, see #1210). Or (using the same options already in place for ck2yaml)

$ cti2yaml haca2.cti --quiet --no-validate

The description is detected and available ...

In [1]: import cantera as ct
   ...: gas = ct.Solution("haca2.yaml")
   ...: print(gas.input_header["description"])
HACA Mechanism - very rough prototype
          Reference:
 (1)  M. Frenklach, H. Wang,
      "Detailed Mechanism and Modeling of Soot Particle Formation"
      in Soot Formation in Combustion: Mechanisms and Models"
      J. Bockhorn Ed., Springer Verlag, Heidelberg (1994).

(note that the odd indent for 'Reference' is preserved from the CTI file pulled verbatim from Cantera 2.5.1)

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
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

Other thoughts

I spot-checked old XML input from 2.5 and there were only two instances where a comment was used, so I do not believe that this capability has to be added to XML (which is usually generated via ctml_writer anyways).

@ischoegl ischoegl marked this pull request as draft February 21, 2022 17:04
@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch from 933779a to 938ee7d Compare February 21, 2022 17:29
@ischoegl ischoegl added the Input Input parsing and conversion (for example, ck2yaml) label Feb 21, 2022
@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch 4 times, most recently from de779ee to 4f71812 Compare February 22, 2022 00:26
@ischoegl ischoegl mentioned this pull request Feb 22, 2022
Open
@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch 3 times, most recently from 5467fc2 to 6d240f5 Compare February 22, 2022 19:42
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Feb 22, 2022 •
edited
Loading

@bryanwweber … I am marking this as ready for review despite some mysterious test failures. As far as I can tell, a Boolean flag is not being passed correctly from the test suite. The ‘interesting’ part is that failures are not repeatable (different runners either pass or fail), and the code itself works as intended. I am stumped …

@ischoegl ischoegl marked this pull request as ready for review February 22, 2022 19:56
bryanwweber
bryanwweber requested changes Feb 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said below, I don't think adding the description to the output needs to be configurable. I find the option name confusing and anyways, it might resolve the test failures 😄

@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch from 6d240f5 to bbbe3ff Compare February 24, 2022 14:01
@ischoegl
[cti2yaml] Convert first comment to description
8bb9568
@ischoegl
[Input] Transfer description from CTI/XML to YAML
4e987ea
@ischoegl
[Samples] Move sample input file to sample/data folder
069f22b 
LiC6_electrodebulk.yaml was the only file in data/sample-data. Moving this file
to samples/data mimics the input file location for the jupyter repository.
@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch from bbbe3ff to 8722780 Compare February 24, 2022 14:09
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Feb 24, 2022

@bryanwweber ... thanks for the suggestions! Omitting the configuration option indeed resolved the pipeline failures (although I still don't know why they happened in the first place).

@ischoegl ischoegl requested a review from bryanwweber February 24, 2022 18:46
@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch from 70ed3d9 to dbf5175 Compare February 28, 2022 17:38
@ischoegl ischoegl mentioned this pull request Feb 28, 2022
Closed
@ischoegl ischoegl marked this pull request as draft February 28, 2022 18:45
@ischoegl ischoegl removed the request for review from bryanwweber February 28, 2022 18:45
@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch from dbf5175 to faf6eaf Compare February 28, 2022 18:47
@ischoegl ischoegl marked this pull request as ready for review March 1, 2022 16:21
@ischoegl ischoegl requested a review from bryanwweber March 1, 2022 16:22
bryanwweber
bryanwweber requested changes Mar 2, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ischoegl A few more changes here. The main ones are the file encoding and multiple blank lines.

If we're deciding not to collect other comment blocks with this PR (which I think is fine) then this shouldn't close the enhancements issue. I think there is still value in attempting to extract those, even if only to try and insert them somewhere in the YAML as a comment, not an actual field. Likewise, I think it's worth trying to do something similar for CTML, which should actually be easier since lxml keeps comment tags.

interfaces/cython/cantera/cti2yaml.py
except ImportError:
print("WARNING: Unable to import Cantera Python module. ")
print("Output mechanism has not been validated")
sys.exit(0)
Copy link
Copy Markdown
Member

@bryanwweber bryanwweber Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perhaps mirroring some other code, but I suspect that sys.exit(1) here would be more appropriate? Also, printing to sys.stderr?

Copy link
Copy Markdown
Member Author

@ischoegl ischoegl Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than not using logger, this is exactly equivalent to what ck2yaml does; as the script could be run without cantera installed on purpose, I think that may have been a deliberate choice. (Aside: ck2yaml prints a single line, which is now also adopted.)

@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch from faf6eaf to 72d58e4 Compare March 2, 2022 14:21
@ischoegl ischoegl force-pushed the detect-description-in-conversion-scripts branch from 72d58e4 to f94d08c Compare March 2, 2022 14:22
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Mar 2, 2022 •
edited
Loading

@bryanwweber ... thank you for the review! I believe everything is addressed. Regarding the ImportError, I just want to keep things consistent with ck2yaml.

If we're deciding not to collect other comment blocks with this PR (which I think is fine) then this shouldn't close the enhancements issue. [...]

I have no issue keeping it open 😃

@bryanwweber bryanwweber merged commit 0dba120 into Cantera:main Mar 3, 2022
@ischoegl ischoegl deleted the detect-description-in-conversion-scripts branch March 3, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bryanwweber bryanwweber bryanwweber approved these changes

Assignees

No one assigned

Labels

Input Input parsing and conversion (for example, ck2yaml)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ischoegl @bryanwweber