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

Added new to_hoomd method #212

Merged
merged 12 commits into from
Feb 20, 2024
Merged

Added new to_hoomd method #212

merged 12 commits into from
Feb 20, 2024

Conversation

janbridley
Copy link
Collaborator

@janbridley janbridley commented Jan 29, 2024

Description

This PR adds a new to_hoomd() method that allows for easy use of coxeter objects with Signac. The intended use case for this is saving a shape to a job document, which allows for simple access to properties important to the simulation.

Motivation and Context

Coxeter works well with simulation tools like hoomd, but it would be beneficial to to have a direct export method. While the gsd_shape_spec is useful for saving simulation data, it does not contain enough data to initialize a simulation in either MC or MD code.

Additional tests have been added to the relevant test files: test_sphere.py::test_to_hoomd, test_ellipsoid::test_to_hoomd, test_polygon::test_to_hoomd, test_polyhedron::test_to_hoomd, and test_spheropolyhedron::test_to_hoomd

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

@janbridley janbridley requested review from a team and DomFijan and removed request for a team January 29, 2024 20:19
@janbridley janbridley self-assigned this Jan 29, 2024
@janbridley
Copy link
Collaborator Author

I looped in @cbkerr on this for some feedback on the signac integration.

coxeter/__init__.py Outdated Show resolved Hide resolved
doc/source/conf.py Outdated Show resolved Hide resolved
Copy link
Member

@cbkerr cbkerr left a comment

Choose a reason for hiding this comment

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

I don't see any problems with the signac aspect, but I didn't run it with signac.

I think the name to_hoomd makes it less discoverable. Although it uses the integrator names of HOOMD, it would be much more broadly useful.

Does this imply that we will add functions like to_lammps in the future? I wouldn't support adding extra overhead to support more backends until the need arises.

What if it were called to_JSON? People using other tools could change the key names as needed and might not think to search for hoomd.

The only thing making it specific to hoomd is that inertia_tensor is mapped to moment_inertia. The to_JSON function should probably keep coxeter's naming, requring hoomd users to change the key.

Extra feature idea: exporting triangulated meshes to rendering tools like fresnel. Not sure what ovito would need. Would this be helpful?

eg I have a function like this where I get the surfaces needed for fresnel.

def tris_for_fresnel(vertices):
    poly = coxeter.shapes.ConvexPolyhedron(vertices)
    t=[i for i in poly._surface_triangulation()]
    tris=[]
    for i in t:
        tris.append(i[0])
        tris.append(i[1])
        tris.append(i[2])
    return tris

coxeter/shapes/convex_spheropolygon.py Outdated Show resolved Hide resolved
coxeter/shapes/convex_spheropolygon.py Outdated Show resolved Hide resolved
coxeter/shapes/convex_spheropolygon.py Outdated Show resolved Hide resolved
coxeter/shapes/convex_spheropolygon.py Outdated Show resolved Hide resolved
coxeter/shapes/convex_spheropolygon.py Show resolved Hide resolved
@janbridley
Copy link
Collaborator Author

janbridley commented Feb 20, 2024

I don't see any problems with the signac aspect, but I didn't run it with signac.

I think the name to_hoomd makes it less discoverable. Although it uses the integrator names of HOOMD, it would be much more broadly useful.

Does this imply that we will add functions like to_lammps in the future? I wouldn't support adding extra overhead to support more backends until the need arises.

What if it were called to_json? People using other tools could change the key names as needed and might not think to search for hoomd.

The only thing making it specific to hoomd is that inertia_tensor is mapped to moment_inertia. The to_JSON function should probably keep coxeter's naming, requring hoomd users to change the key.

Extra feature idea: exporting triangulated meshes to rendering tools like fresnel. Not sure what ovito would need. Would this be helpful?

eg I have a function like this where I get the surfaces needed for fresnel.

My current plan is to have a default to_JSON method that exports any subset of keys to a python dict. The to_hoomd method will call to_JSON and a mapping util that renames the keys to match HOOMD's style. This significantly cuts down on code duplication, and allows for future additions of other mapping functions for more complicated file types - for example, OBJ/OFF files to use with ovito and VTK.

Let me know any thoughts on this approach!

    def to_json(self, attributes: list):
        """Get a JSON-serializable subset of shape properties.
        Args:
            attributes (list):
                List of attributes to export. Each element must be a valid attribute
                of the class.
        Returns
        -------
           dict
                A dict containing the requested attributes.
        Raises
        ------
            AttributeError:
                If any keys in the input list are invalid.
        """
        export = {}
        for attribute in attributes:
            # If an invalid key is passed, raise default attribute error
            export.update({attribute: getattr(self, attribute)})
        return export

Credits.rst Outdated Show resolved Hide resolved
coxeter/shapes/convex_spheropolyhedron.py Show resolved Hide resolved
coxeter/shapes/convex_spheropolyhedron.py Outdated Show resolved Hide resolved
@janbridley
Copy link
Collaborator Author

Once cbkerr approves these changes I will merge and start working on a new release

Copy link
Member

@cbkerr cbkerr left a comment

Choose a reason for hiding this comment

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

Looks great!

@janbridley janbridley merged commit 9652b05 into master Feb 20, 2024
8 checks passed
@janbridley janbridley deleted the to_hoomd branch February 20, 2024 18:52
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.

None yet

3 participants