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

point group of a cluster #43

Open
wants to merge 12 commits into
base: 2.X
Choose a base branch
from

Conversation

seshasaibehara
Copy link
Contributor

@seshasaibehara seshasaibehara commented Jul 29, 2024

  • Migrated the code that I had for point_group_of_molecule from pycasm
  • uses xtal.Occupant to represent a molecule
  • Added tests for cluster_point_group
  • Added a function xtal.Occupant.from_xyz_string() that reads Occupant from xyz file
  • Added tests for xtal.Occupant.from_xyz_string()

@seshasaibehara seshasaibehara marked this pull request as draft July 29, 2024 02:49
@seshasaibehara seshasaibehara marked this pull request as ready for review July 31, 2024 16:30
@bpuchala
Copy link
Collaborator

bpuchala commented Aug 3, 2024

Looks good. A couple changes I'd request:

  • rename cluster_point_group to make_cluster_point_group to match the convention used elsewhere
  • let's use the convention of a single underscore for modules and functions that are not in the public interface. Currently make_cluster_point_group is imported as libcasm.xtal.make_cluster_point_group. So then, unless you want additional methods in the public interface, this would mean renaming cluster_point_group.py as _cluster_point_group.py and adding a leading underscore to the methods other than make_cluster_point_group.

@bpuchala
Copy link
Collaborator

bpuchala commented Aug 3, 2024

One more thing:

  • Are AtomComponent properties or Occupant properties included in the cluster group symmetry analysis? (I don't think they are). If they are not, then maybe we should avoid having them input to make_cluster_point_group and use something else like atom_type: list[str] and atom_coordinate_cart: np.ndarray so it's not a question?

@seshasaibehara seshasaibehara reopened this Aug 9, 2024
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