-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Multi line notes in Chemkin with mixed white space after comment symbols cause issues with write_yaml #1695
Comments
Thanks @corykinney! If I had to guess, this line in |
It looks like this is maybe something that |
If that notation can't be added automatically for the |
The only harm I can think of is that you won't be able to round-trip a chemkin file (meaning ck->yaml->ck) and get identical output. I'm not sure how useful that is, and this seems like the easiest way to fix this particular problem, and having a working YAML file seems more important. |
I'm 👍 for removing the leading whitespace. Preserving this leading whitespace appears to create issues while doing nothing in terms of problem solving. In other words, it's a 'sacrifice' that is inconsequential unless any of us can think of a scenario where 'formatting' chemkin comments is warranted? |
I'm unclear on whether this is a bug in |
Here's a Cantera MWE with the actual error:
with the error being triggered here: Line 1758 in bc24169
It looks like |
I just came across https://github.com/biojppm/rapidyaml, throwing it out there as a potential alternative if we want to go that route. I have no idea if it fixes this bug. |
Performance of that library looks really impressive. At the same time, it’s a rather drastic approach to address this issue 🤣 |
Any updates on resolving this issue? It appears to be the underlying cause of an issue we are seeing in PelePhysics (AMReX-Combustion/PelePhysics#514), where our workflow for pre-processing some mechanisms for use in the Pele reacting flow solvers involves a round trip through |
@baperry2 ... no updates at the moment. It appears that the actual bug is upstream from Cantera in |
Thanks @ischoegl, that makes sense. I agree, once you know what's going on it's not too bad to manually resolve the issue when it comes up, which appears to be fairly rare. |
Preserve indentation of lines within multiline comment blocks, except for the first line which cannot be indented due to limited support for multiline strings from yaml-cpp. Fixes Cantera#1695
Preserve indentation of lines within multiline comment blocks, except for the first line which cannot be indented due to limited support for multiline strings from yaml-cpp. Fixes Cantera#1695
Problem description
Sorry for the title gore, but recently I've been working with the NUIGMech 1.2 and have been having issues performing mechanism reductions and writing the reduced mechanisms to YAML files. The issue seems to stem from the Chemkin source files having comments that have a mixture of whitespace after the exclamation point within the same comment. I'd be happy to help contribute to this, but would need advice on the recommended way to fix it that wouldn't break other behaviors.
Steps to reproduce
Run
ck2yaml
with a Chemkin file with the following reaction, note that the original Chemkin file has a space after the!
in the first line of the comment and no space in the second line of the comment:Produces the following output, which Cantera loads correctly:
The
|2-
keeps the indentation consistent with the first line keeping its extra space when loaded. After loading the mechanism, creating a newct.Solution
object that has that species in it and writing it usingck2yaml
gives the following output:When it gets written, the
2-
is now gone but the initial space gets written out, so this mechanism can't be loaded after the round trip throughct.Solution
and thenwrite_yaml
as either aillegal map value
orend of map not found
error is thrown. If everything else is behaving correctly, maybewrite_yaml
just needs some handling for leading whitespace on multi line notes that theck2yaml
already has? Otherwise, I don't know what the expected behavior is, so I'd appreciate advice on how to go about fixing it. Manually, I've been fixing it by adding the2-
back to reactions that throw errors, but that is a tedious process to do reaction by reaction for such large mechanisms.As a side note, I wish all mechanisms were developed in Cantera in the first place 😜
System information
The text was updated successfully, but these errors were encountered: