WIP: Added instance method to generate MolBar from Structure#236
WIP: Added instance method to generate MolBar from Structure#236crjacinto wants to merge 8 commits into
Conversation
haneug
added
enhancement
|
@crjacinto please repair the title and the description of the PR |
crjacinto
changed the title
crjacinto
changed the title
Done |
haneug
left a comment
haneug
left a comment
There was a problem hiding this comment.
Nice PR! Please split the function according to the return types and change the docstring to numpy style. Also an enum for the mode would be a good thing for verification I think.
crjacinto
changed the title
…y module. Created unit test for Molbar
|
At this point, the latest commit of this code is compatible with the features developed for the RMSD-RotConst implementation (#230). The unit tests are failing since some of the features that are being used now come from the RMSD-RotConst code which has not yet been merged here. Once the other code is merged, I'm going to rebase this commit and it would be ready for review |
… and calculate_molbar_data, modified unit tests.
haneug
left a comment
haneug
left a comment
There was a problem hiding this comment.
Nice work, I would suggest to move the molbar import from the module level to the function level. Also, please add an entry to CHANGELOG.md and I think there was one comment about to_molbar which is not anymore.
|
|
||
| The total charge is taken from `charge`. | ||
|
|
||
| MolBar is **not** a dependency of OPI and must be installed separately:: |
There was a problem hiding this comment.
Here you write molbar is not a dependency and below you write it is optional. We should clearly decide for one thing and go with it. Please make it an optional dependency and pin a version number we want to use.
There was a problem hiding this comment.
I added it as an optional dependency please document it accordingly.
| mode: "MolBarMode | str" = MolBarMode.MB, | ||
| ) -> str: | ||
| """ | ||
| Compute the MolBar barcode string for this `Structure`. |
There was a problem hiding this comment.
Could we add the link to the paper here? With the doi?
| # MolBar is an optional dependency. The decorator below guards every method | ||
| # that calls it and raises a clear ImportError if it is not installed. | ||
| try: | ||
| from molbar.barcode import get_molbar_from_coordinates # type: ignore |
There was a problem hiding this comment.
This makes molbar get imported whenever opi.utils.molbar is imported and since structure.py imports it, that means on any from opi.core import Calculator etc. it will be imported. Molbar is a large package with heavy dependencies, so for users who have it installed this slows down every OPI import even if they never compute a barcode. Suggestion: move the import into call_molbar() so it only happens on first use. Note that requires_molbar currently relies on this module-level import (it checks get_molbar_from_coordinates() is None), so it would need to switch to something like importlib.util.find_spec("molbar").
| def test_topo_mode(self, water): | ||
| """`calculate_molbar_data()` with `mode="topo"` should return the topology barcode.""" | ||
| barcode, _ = water.calculate_molbar_data(mode="topo") | ||
| assert barcode == "TopoBar | 1.1.3 | OH2 | -84 20 344 | 80" |
There was a problem hiding this comment.
can we replace 1.1.3 by molbar.__version__? Is that possible?
There was a problem hiding this comment.
When I looked at molbar.__version__) it raised an AttributeError
3 participants
Closes Issues
Closes #227
Description
Release Notes
(Project adheres to Keep a Changelog; Every entry should start in upper-case and end with
(#<pr-id>); Remove sections that don't apply)Changed
structure.py.