Skip to content
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

Ion bugfixes and enhancements #2287

Merged
merged 9 commits into from
Nov 9, 2021
Merged

Conversation

rkingsbury
Copy link
Contributor

@rkingsbury rkingsbury commented Oct 31, 2021

Summary

This PR contains some miscellaneous bugfixes and enhancements for the Ion class.

  • Fixes Ion.from_formula has unexpected behavior when +/- precede charge number #2281
  • Partially address Inconsistency between formula and reduced_formula for diatomic molecules and Ions #2204 by overloading get_reduced_formula_and_factor to provide special formula handling for ions. The reduced_formula for hydroxide, hydrogen peroxide, acetate, acetic acid, and small alcohols are now more consistent with the way these species are usually written.
  • Implement a utils.string.charge_string method to make the representation of ion charges more consistent across formula, reduced_formula, etc.
  • Make representation of Ion formulae more consistent. By default, charges are now always appended in brackets, with the sign preceding the magnitude, e.g. Na[+1] or SO4[-2]. This provides unambiguous differentiation between stoichiometric coefficients and charges, and will facilitate using Ion.reduced_formula as database keys in, e.g., the Pourbaix ion data. For uncharged Ion, the formula is always followed by (aq).
  • Change display of ions that may be hydrated metal complexes, e.g. Zr(OH)4(aq) becomes ZrO2.2H2O(aq)
  • Add extra tests for to_latex_string and Ion.from_formula
  • Add type hinting

Additional dependencies introduced (if any)

None

Checklist

  • Code is in the standard Python style. The easiest way to handle this
    is to run the following in the correct sequence on your local machine. Start with running
    black on your new code. This will automatically reformat
    your code to PEP8 conventions and removes most issues. Then run
    pycodestyle, followed by
    flake8.
  • Docstrings have been added in the Google docstring format.
    Run pydocstyle on your code.
  • Type annotations are highly encouraged. Run mypy to type check your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

@coveralls
Copy link

coveralls commented Oct 31, 2021

Coverage Status

Coverage decreased (-0.6%) to 83.115% when pulling d378d27 on rkingsbury:ion_fixes into acfe589 on materialsproject:master.

@mkhorton mkhorton merged commit 39612af into materialsproject:master Nov 9, 2021
@mkhorton
Copy link
Member

mkhorton commented Nov 9, 2021

Thanks for this PR @rkingsbury ! looks great.

One concern is the proliferation of special cases for organics, I wonder if there's a better long-term solution to this that's more general/transferrable.

rkingsbury added a commit to rkingsbury/mdgo that referenced this pull request Nov 23, 2021
See pymatgen #2287
materialsproject/pymatgen#2287

The data file is now keyed by
Ion.reduced_formula, meaning all
ions have charges appended in
brackets, with explicit magnitudes
e.g. `Li[+1]`
@rkingsbury rkingsbury deleted the ion_fixes branch June 8, 2022 22:25
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.

Ion.from_formula has unexpected behavior when +/- precede charge number
3 participants