Skip to content

Yaml writer addition #2800

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Yaml writer addition #2800

wants to merge 6 commits into from

Conversation

rwest
Copy link
Member

@rwest rwest commented May 30, 2025

Motivation or Problem

Previously, RMG created CHEMKIN files as outputs when generating a mechanism. The final CHEMKIN file for a mechanism is then converted to a CTI file, which can be used in Cantera simulations. When dealing with more complex mechanisms, RMG has a problem converting the final CHEMKIN file to a CTI file (because of numerous unmarked duplicate reactions in CHEMKIN file). To fix this issue, the CHEMKIN file has to be manually edited before file conversion can take place. This is time-consuming and tedious. Additionally, CTI input files are depreciated in Cantera 2.5 and will be removed in Cantera 3.0. For those using Cantera, it would be beneficial if RMG could directly output Cantera-formatted YAML files.

The goal is to natively write cantera-compatible YAML files

Description of Changes

This renames rmgpy.yml module to be rmgpy.yaml_rms and adds a rmgpy.yaml_cantera.
Currently it functions by creating Cantera objects for things in memory, and having Cantera turn them into appropriate yaml.
Benefit is the "don't repeat yourself" approach. If we already want to maintain a direct-to-cantera-object mapping, and cantera already maintain an object-to-yaml mapping, then we could just re-use both, ensuring all the parts stay working.
Drawback is probably inefficiency. We could probably just use string templates for many of these cases and make it a whole lot faster.

Testing

There's a new test, as part of this PR.

History/Context

For context, can refer do discussion on these:
#2321 Created a YAML writer.
#2770 Created some tests for it.

Both PRs got a bit messed up with force pushing their branches, so are closed and archived.
The commits now live in this PR

rwest and others added 6 commits May 30, 2025 10:27
…lder.

So they don't over-write the ones that are being written directly.
This allows the Cantera yaml writer to live alongside it
Nora did most of the work.
Richard did some cleanup.
Nick fixed species_to_dict function to use correct parameter as rxn species.

Co-authored-by:  Nora Khalil <[email protected]>
Co-authored-by: Richard West <[email protected]>
Co-authored-by:  Nicholas Tietje <[email protected]>
Although Atom is a subclass of Vertex, it adds an element
attribute. This leads to more efficient Cython code
(and quietens a warning in vscode).
Basing it on the Chemkin version. 
For now only evaluate it once, and include everything.
@rwest rwest requested a review from Copilot May 30, 2025 20:32
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds native YAML output support for Cantera, renames the existing RMS YAML writer module, and introduces tests to validate YAML outputs.

  • Renamed rmgpy.ymlrmgpy.yaml_rms and updated function names (write_ymlwrite_rms, convert_chemkin_to_ymlconvert_chemkin_to_rms).
  • Added a new Cantera-compatible YAML writer in rmgpy.yaml_cantera.
  • Introduced tests and a comparison utility (CompareYaml) for validating species and reaction data between RMS and Cantera YAML outputs.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/rmgpy/yaml_writer/test_yaml.py Added pytest fixtures and tests for comparing YAML data
test/rmgpy/yaml_writer/compare_yaml_outputs.py New utility to load, normalize, and compare YAML outputs
rmgpy/yaml_rms.py Renamed functions for RMS YAML output and updated calls
rmgpy/yaml_cantera.py New module: writes RMG model to Cantera-compatible YAML
rmgpy/rmg/main.py Updated imports and attached CanteraWriter listener
rmgpy/reaction.py Updated Cython declares to use Molecule/Atom types
Comments suppressed due to low confidence (3)

rmgpy/yaml_rms.py:52

  • convert_chemkin_to_rms should call write_rms instead of write_yml since write_yml was renamed to write_rms.
write_yml(spcs, rxns, path=output)

test/rmgpy/yaml_writer/compare_yaml_outputs.py:96

  • [nitpick] Hard-coding phase positions for gas and surface counts is brittle; consider iterating over the phase count dictionaries to compare all phases dynamically.
count_diff = {'gas': count_per_phase1[f"specie_count_{phase_names1[0]}"] - count_per_phase2[f"specie_count_{phase_names2[0]}"] ... }

test/rmgpy/yaml_writer/compare_yaml_outputs.py:8

  • [nitpick] Hard-coded test file paths make tests fragile; consider constructing paths relative to the test directory using pathlib or pytest fixtures.
    yaml_files = { 'yaml1': ['RMG_yaml_writer_addition/RMG-Py/test/rmgpy/test_data/...'] }

@@ -0,0 +1,23 @@
from compare_yaml_outputs import *
Copy link
Preview

Copilot AI May 30, 2025

Choose a reason for hiding this comment

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

[nitpick] Avoid using wildcard imports in tests; explicitly import CompareYaml to improve readability and avoid namespace collisions.

Suggested change
from compare_yaml_outputs import *
from compare_yaml_outputs import CompareYaml

Copilot uses AI. Check for mistakes.

@rwest
Copy link
Member Author

rwest commented Jun 1, 2025

I expected there would be problems.

During the regression tests we get a Segmentation fault (core dumped), minimal_surface Failed to Execute.
Strangely minimal_surface does not use RMS (though it still builds julia stuff), and some of the previous RMS simulations seemed to work. And it runs fine on my intel macbook pro.

Did the segfault's that @JacksonBurns ran into with Cantera/RMS conflicting in #2741 look similar to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants