Replace site.species_and_occu with site.species property for compatible pymatgen versions

[astamminger]

Fixes #3297

Added check for the used pymatgen version and dynamically switch to Site.species property if applicable (i.e. pymatgen>=2019.3.13). Prior to 2019.3.13 the old Site.species_and_occu property is used.

[greschd]

Thanks for taking care of this! 😃 Looks good overall.

[greschd]

I think here it'd be better to just do plain string comparison - something like "2019.3.13b2" would also be a legal version string (although pymatgen doesn't seem to use it). That would break the int conversion above.

Sorry for the nit-pick 😬

[sphuber]

Better still I would say is to use something like:

from distutils.version import StrictVersion
current_version = StrictVersion(get_pymatgen_version())
required_version = StrictVersion('2019.3.13')
if current_version < required_version:
    species_and_occu = site.species_and_occu

[greschd]

Nice 👍

Btw, do we want to bump the pymatgen version in setup.json? It's currently at "pymatgen<=2018.12.12". Not sure what the differences are though, tbh.

This issue came up because some dependency of mine changed the pymatgen version.

[sphuber]

We can't unfortunately because they dropped python 2 support after that.

[greschd]

Soon... 😎

[greschd]

I think you can ignore the coverall failure, but I'll leave that to @sphuber to confirm.

[sphuber]

Thanks for the update @astamminger . One question: is there a reason you prefer the dependency packaging over the standard library distutils?

[astamminger]

Was about to write a comment on that but you were too fast :D Mainly the reason was that AFAIK StrictVersion provided by distutils does not comply to PEP440 so I thoughtpackaging might be a better alternative (But I'm happy to change that if you prefer to use distutils)

[sphuber]

Thanks, I didn't know that. It seems indeed that packaging seems the more reliable option. I was just worried about it being third-party and therefore adding an implicit dependency, but since it gets installed by setuptools that is ok

[greschd]

From looking at the list of maintainers on PyPI, packaging seems pretty solid. We should add it as an explicit dependency in setup.json if we use it explicitly, though.

[sphuber]

Could you add the dependency to the atomic_tools extra @astamminger ? Since it is only used in combination with pymatgen I think it is ok to just put it in that extra

[greschd]

Shouldn't we move the import to the function itself then? Might be easier to just put it in the general dependencies.

[astamminger]

We could also use pkg_resources.parse_version() which is distributed with setuptools so we can avoid adding an additional dependency plus being PEP440 conform. What do you think?

// EDIT: No idea what's going on with the coveralls, though!

[greschd]

Go for it 👍 Let's try not to bike-shed too hard here..

I have a feeling the coveralls can be a bit flaky sometimes.

[sphuber]

@astamminger if you can move the pkg_resources import within the pymatgen function, we do not have to add it to the explicit dependencies. If you can do that final thing I will approve and merge this, thanks!

[sphuber]

Thanks @astamminger