update parser to handle hydrates, rational subscripts, complexes, and caged species

[jeremyagray]

Update parser to handle hydrates (via the .. operator, not .), rational subscripts (subscripts as decimals, with .), coordination complexes (uses grouping symbols (), [], and {} but does not attach special meaning), and caged species (via the @ operator). Add parsing of some state symbols to the main parser, but suppress them for now so that the phase identification code in chemistry.py does not need changed; this does not affect the existing state symbol handling. Update parsing tests to reflect changes and make minor updates to the rest of the code and other tests to accommodate parser changes. Implement the planned deprecation of / in charge strings.

Breaking change: Breaks the old hydrate operator, .. Hydrates must now be indicated with the .. operator, as in BaCl2..2H2O. The old hydrate operator, ., is now used for rational subscripts. As warned in the last deprecation cycle, / is now invalid in charge strings.
Closes: closes #202, #176

[bertiewooster]

Great, thanks @jeremyagray! I just visually verified that some tests we discussed (@ cage operator, .. hydrate operator, and non-integer subscripts) were included, and they are. A couple thoughts on exceptions testing:

• Would it be worth adding exceptions testing for negative subscripts and charges, and fractional charges, for example Ca-2 and Ca-2.832 (which would presumable be interpreted as a charge) and Ca+2.832?
• If there are multiple statements in a with pytest.raises(): block, such as:

@requires(parsing_library)
def test_formula_to_composition_deprecated_charge():
    with pytest.raises(ValueError):
        # Ions.
        formula_to_composition("Cl/-")
        formula_to_composition("Cl/-(aq)")

I did a check for a simple case, and it doesn't seem to require all of the statements to raise that error. For example, this test passes:

def test_raises():
    with pytest.raises(ZeroDivisionError):
        9 / 0
        2 / 1

despite the second division statement not raising ZeroDivisionError. So unless I'm missing some context (I might be!), I think each of those checks might need to be done individually. To continue the good practice of avoiding re-writing the test code, the test can be parametrized, for example like this:

@requires(parsing_library)
@pytest.mark.parametrize(
    "formula",
    [
        ("Cl/-"),
        ("Cl/-(aq)"),
    ],
)
def test_formula_to_composition_deprecated_charge(formula):
    with pytest.raises(ValueError):
        formula_to_composition(formula)

[jeremyagray]

No, @bertiewooster you're right about the tests. I was just adding tests for this PR without much thought for organization as I planned to do another PR on some testing issues that needed attention. All the parsing tests and probably some others should be parameterized and I thought to do that and inspect the coverage in another issue/PR.

If there are no other problems with this PR, one of us should probably get around to merging it in the not to distant future.

[bertiewooster]

Hi @jeremyagray is there anything else that needs to be done to push all your hard work across the finish line and justify a new release, or does it just need to be merged?

[jeremyagray]

No, not that I know of. I just left it to allow (hopefully) someone else to verify it works before I finally push it to the main branch.

…

On Sat, Mar 12, 2022 at 11:16 AM Jeremy Monat ***@***.***> wrote: Hi @jeremyagray <https://github.com/jeremyagray> is there anything else that needs to be done to push all your hard work across the finish line and justify a new release, or does it just need to be merged? — Reply to this email directly, view it on GitHub <#205 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOQCHS7LJHOEODMH2X33XZLU7TGOJANCNFSM5N4FDHJQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Jeremy A. Gray

[bertiewooster]

@jeremyagray Could you parametrize the tests which contain
with pytest.raises(...

so that each parameter set is tested, that it actually fails, and not just one parameter set? I believe that should be done for the following:

• def test_formula_to_composition_fail(): currently has four raises blocks
• def test_formula_to_composition_deprecated_charge(): currently has 11 parameter sets
• def test_formula_to_composition_bad_complexes(): currently has three raises blocks

[jeremyagray]

I just parametrized all the parsing tests. They probably need some more organization, but this at least gets all the tests running as they should and has the added benefit of boosting the test statistics.

The checks are failing because I've got some version and configuration drift between my local black and flake8 and the CI machine's versions. I'll fix that shortly.

[bertiewooster]

I visually reviewed the changes and they seem good, so once the checks pass, I recommend merging this commit. I like the elegance of the tests now that they're parametrized, for example
assert formula_to_composition(species) == composition

Thanks for putting in the extra effort to parametrize all the tests!

[bertiewooster]

Changes seem fine and checks passed. Unless @jeremyagray has any more changes in the works, recommend merging.

[bjodah]

Some really nice work here, thank you @jeremyagray for getting this done!