Cantera / cantera

Chemical kinetics, thermodynamics, and transport tool suite
https://cantera.org
Other
580 stars 341 forks source link

yaml2ck fails for phases with no reactions #1679

Open speth opened 3 months ago

speth commented 3 months ago

Problem description

Trying to convert a YAML mechanism to CK format fails when the phase has no reactions.

Steps to reproduce

  1. Download attached org.yaml.txt and save as org.yaml
  2. From a command prompt, run yaml2ck --mechanism=chem.inp --thermo=therm.dat --overwrite org.yaml

Behavior

yaml2ck fails with the following error:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "R:\ray\mamba\ctdev312\Lib\site-packages\cantera\yaml2ck.py", line 856, in <module>
    main()
  File "R:\ray\mamba\ctdev312\Lib\site-packages\cantera\yaml2ck.py", line 807, in main
    output_paths = convert(
                   ^^^^^^^^
  File "R:\ray\mamba\ctdev312\Lib\site-packages\cantera\yaml2ck.py", line 704, in convert
    mechanism_text.append(build_reactions_text(all_reactions))
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "R:\ray\mamba\ctdev312\Lib\site-packages\cantera\yaml2ck.py", line 356, in build_reactions_text
    max_reaction_length = max(len(r.equation) for r in reactions)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: max() iterable argument is empty

System information

Attachments

org.yaml.txt

Additional context

Originally reported on the Cantera Users' Group

corykinney commented 2 weeks ago

@speth would the desired behavior be to simply skip the build_reactions_text call if n_reactions (or in this case just len(all_reactions)) for the phase is zero? If so, I can implement a quick fix.

speth commented 2 weeks ago

Yeah, that sounds like it would be sufficient. Or I guess, the most Pythonic check would be if all_reactions: ....

I just noticed the comment that @bryanwweber had put in where this is called:

# TODO: Handle phases without reactions
bryanwweber commented 2 weeks ago

And this is why TODO comments aren't helpful 🤣🤣🤣

corykinney commented 2 weeks ago

I need to go ahead and add a test case. I've already confirmed the fix works for roundtripping with ck2yaml, but does Chemkin require a REACTIONS section to be present or is it okay if it's missing? The barebones example I tested with the fix referenced gives a reactions file like so:


ELEM
H
END

SPECIES
H2
END

If Chemkin requires it, I could modify build_reactions_text to at least do:

REACTIONS
END

but I figured I'd ask since I'm only somewhat familiar with Chemkin's file requirements.

speth commented 2 weeks ago

Yeah, that's a test we can't easily run. I'd be inclined to assume that the REACTIONS section is optional.