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

fix valence bug, add test #1926

Merged
merged 1 commit into from
Aug 12, 2020
Merged

Conversation

rkurchin
Copy link
Contributor

Previously, asking for the valence property on an Element with full shells that wasn't noble (e.g. column 2 or 12) would cause an error. This resolves that.

Summary

Include a summary of major changes in bullet points:

  • Added a check for if the last orbital listed is fully-occupied and to choose that number as the valency
  • Added a case to the tests to trigger this

Additional dependencies introduced (if any)

none

Checklist

Before a pull request can be merged, the following items must be checked:

  • [ x ] Code is in the standard Python style.
    Run pycodestyle and flake8
    on your local machine.
  • [ x ] Docstrings have been added in the Google docstring format.
    Run pydocstyle on your code.
  • [ x ] Type annotations are highly encouraged. Run mypy
    to type check your code.
  • [ x ] Tests have been added for any new functionality or bug fixes.
  • [ ? ] All existing tests pass. --> I ran the test_periodic_table.py manually, but nosetests continually hangs after 1089 tests. I installed all dependencies (including optional and dev ones) in my environment so I'm not sure what the issue is with that, but I can't imagine what this would break that's not covered by the test I checked.

Previously, asking for the valence property on an Element with full shells that wasn't noble (e.g. column 2 or 12) would cause an error. This resolves that.
@mkhorton
Copy link
Member

Many thanks @rkurchin !

@mkhorton
Copy link
Member

On the test note, I agree this is unlikely to fail but I'll let the tests pass on CI first before merging. Your issue might be related to using nose which is now deprecated, pytest should be sufficient to run tests.

We're also asking new contributors to fill out this form so that everyone can be acknowledged appropriately in the pymatgen documentation.

@rkurchin
Copy link
Contributor Author

Got it – might want to update the instructions here then, that's why I used nose, even though I saw on its docs page that it was deprecated.

For my own stuff (since I need this to work properly for something I'm doing), I imagine once this is merged in I can expect it in the version I get from pip install within a couple days? In the meantime I can just use my local version obviously.

@mkhorton mkhorton merged commit d8da524 into materialsproject:master Aug 12, 2020
@mkhorton
Copy link
Member

Thanks for the catch! Yes that page is definitely out of date, I've pushed some minor edits to the documentation for now but we're over-due for a more thorough documentation update.

We don't currently have a fixed schedule for pymatgen releases, we usually make one or two per month as pull requests come in or on an as-needed basis. I'll see if I can get this released by the end of the week.

@mkhorton
Copy link
Member

FYI, this was released as part of 2020.8.13

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.

None yet

2 participants