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

add kwarg to MoleculeGraph method and fix PackmolSet bug #2927

Merged
merged 8 commits into from
Apr 4, 2023

Conversation

orionarcher
Copy link
Contributor

@orionarcher orionarcher commented Apr 1, 2023

Summary

The get_disconnected_fragments method returns a new graph with reordered indices but provides no way to get the mapping from new to original indices. This PR reconstructs that map and adds an optional keyword argument to return it. This will not affect current functionality.

Additionally, I found an error in the pymatgen.io.packmol code where a non-abstract class had an abstract method, preventing it from being instantiated. This slipped under the radar because the tests are skipped due to a difficult dependency (packmol). The tests are now passing locally for me.

A downstream code I am working on requires this addition.

  • a new return_index_map kwarg for MoleculeGraph.get_disconnected_fragments
  • remove abc.abstractmethod decorator from PackmolBoxGen.get_input_set()

Checklist

  • Doc strings have been added in the Google docstring format. Run pydocstyle on your code.
  • Type annotations are highly encouraged. Run mypy path/to/file.py to type check your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

@orionarcher orionarcher changed the title Add a keyword to pymatgen.analysis.graphs.MoleculeGraph.get_disconnected_fragments add kwarg to MoleculeGraph method and fix PackmolSet bug Apr 3, 2023
@janosh janosh enabled auto-merge (squash) April 4, 2023 14:34
@janosh
Copy link
Member

janosh commented Apr 4, 2023

Thanks @orionarcher! 👍

@orionarcher
Copy link
Contributor Author

Thank you @janosh!

@janosh janosh merged commit 1112d6d into materialsproject:master Apr 4, 2023
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.

2 participants