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

Int now valid for get_indices #38

Merged
merged 8 commits into from
Nov 5, 2023
Merged

Int now valid for get_indices #38

merged 8 commits into from
Nov 5, 2023

Conversation

arm61
Copy link
Collaborator

@arm61 arm61 commented Oct 24, 2023

Added parsing for interger indexes to the get_indices function of the MDAnalysisParser class

Added parsing for interger indexes to the get_indices function of the MDAnalysisParser class
Copy link
Collaborator Author

@arm61 arm61 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good. I guess that your editor is automatically applying some formatting with shorted line lengths than we typically use (in kinisi I think we have been using 120 characters as the 79 of PEP 8 feels a bit on the short side).

@staticmethod
def get_molecules(structure: "ase.atoms.Atoms", coords: List[np.ndarray],
indices: List[int]) -> Tuple[np.ndarray, np.ndarray, Tuple[np.ndarray, np.ndarray]]:
"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is still a docstring missing here.

kinisi/parser.py Outdated
return structure, coords, (flat_indices, framework_indices)

@staticmethod
def get_framework(structure: "ase.atoms.Atoms", indices: List[int]) -> Tuple[np.ndarray, np.ndarray]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here, I guess it is just about copy and pasting from the other parsers.

kinisi/parser.py Outdated

structure, coords, latt = self.get_structure_coords_latt(atoms, sub_sample_traj, progress)
progress: bool = True,
s_indices: List[int] = None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is short for specie_indices right? Perhaps just indices or the fully specie_indices is preferred?

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