WIP: Added instance method to generate MolBar from Structure#236
WIP: Added instance method to generate MolBar from Structure#236crjacinto wants to merge 3 commits into
Conversation
|
@crjacinto please repair the title and the description of the PR |
Done |
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.
|
|
||
| Returns | ||
| ------- | ||
| str |
There was a problem hiding this comment.
Please split this into two separate functions, one returning a str, one returning tuple[str, dict[str, Any]. A function that changes its return type based on a keyword argument makes type hints unreliable and is easy to misuse. Maybe make to_molbar return the string and to_molbar_dict or to_molbar_full return the whole data.
Also please document or give the link to the documentation what this whole data even means.
| elements.append(parts[0]) | ||
| coords.append([float(parts[1]), float(parts[2]), float(parts[3])]) | ||
|
|
||
| if not elements: |
There was a problem hiding this comment.
You could check for if self.atoms before trying to loop over them and then the error message would fit better.
…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 |
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.