Fix #853 raise an error when there is number in an element symbol during ck2yaml conversion

[12Chao]

Problem:

Regarding to #853, When there is large element number(more than 3 digits) in the thermo file trying to fit everything in one line, instead writing an additional line with element numbers like this:

cantera/test/data/soot-therm.dat

Lines 39 to 49 in 7d87534

	BIN6J PYRENEJ1 C 0H 0 0 0G 300.000 5000.000 1401.000 01&
	C 778 H 263
	3.63345177E+01 3.13968020E-02-1.09044660E-05 1.71125597E-09-1.00056355E-13 2
	4.05143093E+04-1.77494305E+02-1.20603441E+01 1.59247554E-01-1.41562602E-04 3
	6.26071650E-08-1.09305161E-11 5.56473533E+04 7.68451211E+01 4
	BIN6 PYRENE C 0H 0 0 0G 300.000 5000.000 1401.000 01&
	C 778&
	H 264
	3.65839677E+01 3.36764102E-02-1.16783938E-05 1.83077466E-09-1.06963777E-13 2
	9.29809483E+03-1.81272070E+02-1.29758980E+01 1.63790064E-01-1.43851166E-04 3
	6.31057915E-08-1.09568047E-11 2.48866399E+04 7.94950474E+01 4

The ck2yaml file could accidentally convert an incorrect element symbol in species composition, which includes part of the element number in the element symbol string for example: The composition for
BIN7AJ C 1250H 812 G 300.00 3500.00 1260.00 1 is parsed as {'1': 250, H: 812} It won't pass the validation, but raise an error before a yaml file gets created maybe more helpful.

Fix:

Check if there is any number in the symbol and if the symbol is in the elements list when creating the element symbol. If the check failed, raise the error thermo entry and mention the first line has an incorrect format.
Checklist

• There is a clear use-case for this code change
• The commit message has a short title & references relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• The pull request is ready for review

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

Fixes #853

Changes proposed in this pull request

raise an error including the entire thermo entry when parsing a species thermo data with big element number in incorrect format

[bryanwweber]

Thanks for this change @12Chao! It's a great start. I have a few comments below. One additional comment is, rather than including a large mechanism, we'd rather have the smallest possible mechanism that can reproduce the error condition. You can have it as small as one species, and even put the thermo data into the input file.

[bryanwweber]

The way Python works, you don't need the == True, just any(map(str.isdigit, symbol)) should be enough.

[12Chao]

Hi @bryanwweber, thanks for the comments! I rebased the commit, and I think I have got all the changes you mentioned earlier in the comments. Please let me know if I should change anything.

[bryanwweber]

Hi @12Chao I made a few more changes to simplify things a bit and change the comment to clarify what I meant about explaining why 😄 Let me know what you think!

[12Chao]

Thanks @bryanwweber! I think these changes really help make thinge clear. One thing I am not sure is that should we mention that there is a specified chemkin format about how to add large element number in the thermo entry. I feel like that is helpful but it will probably affect the simplicity of the error message.

[bryanwweber]

@12Chao That's a good point. I can't quite think of a better way to phrase it to point out there is an alternate syntax, without, as you say, removing the simplicity of the error message. It's not that elements can't have more than 3 digits, it's that they can't have more than three digits when specified in the default location. But I don't know how to point that out in a way that is useful for the end user.

[speth]

Would it be helpful to include the URL to our help on debugging issues in Chemkin files (https://cantera.org/tutorials/ck2yaml-tutorial.html#debugging-common-errors-in-ck-files)? One flaw with this option is that our instructions there don't currently provide any help for this type of error, and don't describe the supported syntax for extended composition information, but those are things that you could fix.

[12Chao]

I think it is a good option, and the syntax for extended composition information is something good to have on the Cantera website no matter if we add url to the error message or not. I can work on adding the instruction about that syntax on Cantera website, and it would be nice to add the url to other error messages raised due to the indentation error or other format errors so that people can compare the format there as a template.

I am thinking that would it be helpful if we add a flag like --verbose to enable an explaination about an error and the possible fix?

[speth]

Thanks, @12Chao, adding that documentation would be helpful. And adding links to other error messages would be helpful as well.

If you want to avoid repeating the URL in many places, you could just assign it to a global variable and substitute that into the error messages.

I wouldn't worry too much about adding a --verbose flag -- I think it would be appropriate to provide this information by default.

[speth]

@12Chao, Can you rebase this on to the current main branch so it will run the new CI tests (added in #775)?

[12Chao]

@12Chao, Can you rebase this on to the current main branch so it will run the new CI tests (added in #775)?

Sorry, I guess that I just messed it up, and I will open a new pull request about this change

[speth]

I don't think you need to close this PR. All you need to do is push a branch that contains the current main plus your 1 commit to this same branch name.

[12Chao]

Thanks @speth, I just pulled branch 'main' and pushed it with the commit to the same b
ranch again

[bryanwweber]

I believe I've fixed the merge conflicts that were happening and reset this branch to the correct state, rebased onto the current tip of main.

I have one suggestion here, which is that since the same message is added to every InputError, why not just add it to that class? Something like:

class InputError(Exception):
    def __init__(self, message, *args, **kwargs):
        message += "\nPlease check https://cantera.org/... for the correct Chemkin syntax."
        if args or kwargs:
            # the rest of the method is the same

Then, each time InputError is called, just make sure the existing message ends with a period so that the grammar works out.

[bryanwweber]

By the way @12Chao, to get your branch the same as this one, do

git fetch origin
git reset --hard origin/improve_ck2yaml

The first command updates the reference for this branch, and the second one resets your working directory and history to match the updated reference. Just make sure you there are no uncommitted changes in your folder before you run the second one.

[12Chao]

Thanks for the fix @bryanwweber, that's really helpful. I have added the message with url to the InputError.

[12Chao]

Thanks @speth for the revision. I have pulled the latest version of Cantera and made the suggested changes, but it seems ruamel-yaml cannot be built successfully on system with Python3.8 on Travis. I am not sure how to fix that, and it would be really appreciated if you can give me some hints about how to fix that

[speth]

@12Chao, the revision I made was mostly to get rid of the merge commit and to combine a few commits which were just modifying the same bits of code. As a reminder for the future, please do not merge changes from the main branch into your PR branch. If you want to update your PR, rebase your branch onto the head of the current main branch.

The test failures have nothing to do with any of your code, so it isn't necessary for those to pass for this PR to be merged. See #924 for what we currently know about those test failures.

[12Chao]

Thanks @speth, I will do what you said on another pull request I made.