bjodah / chempy

⚗ A package useful for chemistry written in Python
BSD 2-Clause "Simplified" License
544 stars 78 forks source link

Option to use NIST periodic table data #175

Closed jeremyagray closed 4 years ago

jeremyagray commented 4 years ago

This will restructure chempy/util/periodic.py to use elemental data files in chempy/util/data to load the original (and now fixed) or NIST data depending on user preference. This also includes some additions and reorganization of the ions in support of another branch where I am implementing some of the parsing of ions (and complexes) that has been stubbed in chempy/util/_aqueous.py. In line with the contribution guidelines, I have also began some linting (PEP 8, 257) in chempy/util/tests (another branch).

I've sat on this fix for #171 and #172 since around February and really should have pushed it before having to rebase twice on a Sunday afternoon.

bjodah commented 4 years ago

Hi Jeremy, thank you for working on this! Glad to see you're taking the time to contribute.

I totally feel you, been there many times myself. :). And I just want to check: can you see the CI log? (it's on a private server, so I'm never quite sure what viewing restrictions apply to others than myself).

I'll have a look on your code and will get back to you here.

bjodah commented 4 years ago

@matecsaj feel free to chime in here as well.

matecsaj commented 4 years ago

@bjodah, thanks for the invite. Unfortunately, I don't have the required expertise. Best wishes.

jeremyagray commented 4 years ago

On Sun, Jul 5, 2020 at 4:49 PM Bjorn notifications@github.com wrote:

As for the CI data, I found it. Looks like the CI server is running flakes and I was using pytest/pycodestyle/pydocstyle locally. I'll get flakes installed locally and squash its complaints.

@bjodah commented on this pull request.

OK, I've skimmed thought left some minor comments.

A couple of things come to mind:

  1. What are the license / usage conditions for the NIST data? Are we free to redistribute? Do we need to add some license/copyright file?
  2. I'd rather not store state in the python package, i.e. have the user call some activate and deactivate function for enabling/disabling NIST data, or is this just internal API? I'm fine with dropping the older data and just migrating to NIST data if it's preferable to the wikipedia data I've been referencing. We can bump the ChemPy version number to indicate a breaking change if need be.
  3. For isotopes, I did look into this a while back, I took note of pyne https://pyne.io/pyapi/data.html, but at the time it was quite tricky for users to install so maybe it's worthwhile having some isotope representation in ChemPy.
  4. Are the Atom, AtomicIon & Isotope classes integral to the migration to using NIST data? If not perhaps we could review those in a separate pull request if that's alright with you?
  1. NIST data is free unless copyrighted by someone else ( https://www.nist.gov/disclaimer). Disclaimer is appreciated but not required so something more than the passing reference in periodic.py may be appropriate.
  2. I agree; I just used that as a default to keep backwards compatibility. 3 & 4. The new classes aren't strictly necessary for the NIST data and can be (re)moved easily. They were the easy part of solving the isotope problems with the transuranium elements and a plan to implement a list of isotopes, masses, and methods to calculate average atomic masses from https://www.chem.ualberta.ca/~massspec/atomic_mass_abund.pdf, with caveats about the actual copyright of that data. With that data, it would not be a big deal to implement those classes so that an isotope of a given element defaults to its most abundant isotope (if known) and automatically updates its data if a different isotope is selected. So this should be a new branch.

In chempy/util/_aqueous.py https://github.com/bjodah/chempy/pull/175#discussion_r449921011:

 'ClO-': 'hypochlorite',

'ClO2-': 'chlorite', 'ClO3-': 'chlorate', 'ClO4-': 'perchlorate',

  • 'CrO4-2': 'chromate(VI)',
  • 'BrO3-': 'bromate',
  • 'IO3-': 'iodate',
  • 'SiO3-2': 'silicate',

isn't this one called metasilicate?

Laziness typo. The meta- always seems to get dropped in conversation.


In chempy/util/_aqueous.py https://github.com/bjodah/chempy/pull/175#discussion_r449921057:

 'FeO4-2': 'ferrate(VI)',
  • 'MnO4-': 'permanganate(VII)',
  • 'MnO4-2': 'manganate(VI)',
  • 'NO2-': 'nitrite',
  • 'NO3-': 'nitrate',
  • 'OH-': 'hydroxide',
  • 'O2-': 'peroxide',

looks like superoxide to me?

My brain either read the 2 twice for peroxide or thought oxide. But yes. Or maybe I can't read.

-- Jeremy A. Gray

jeremyagray commented 4 years ago

I'm going to close this and reorganize. I've got too much extraneous stuff here and somehow pulled this from my master.