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

Add support for creation of box object from non-triangular matrices #1769

Merged

Conversation

DomFijan
Copy link
Contributor

@DomFijan DomFijan commented Apr 23, 2024

Description

Current implementation of hoomd's box.from_matrix only supports upper triangular matrices, and the class is lacking support for creating boxes from general cell matrices. This PR should enable this. This should be a non-breaking change since upper triangular matrices should still work in the same manner.
EDIT:
A new method box.from_basis_vectors was added instead of changing the functionality of from_matrix. This leaves freud inconsistency unresolved.

Motivation and context

Freud's box class has a from_matrix that supports both (upper) triangular matrices as well as other general cell matrices. On previous dev-meeting we discussed that this behavior should be the same between the two codes and that hoomd's box.from_matrix should be updated to also support box matrices that are not upper triangular ones.

How has this been tested?

I have added a test which tests creation of box object from a valid cell matrix that is not an upper triangular one.

Change log

*Added*

* ``hoomd.Box.from_matrix`` - Added support for general cell matrices.

Checklist:

@DomFijan DomFijan marked this pull request as ready for review April 23, 2024 20:11
@DomFijan DomFijan requested review from a team as code owners April 23, 2024 20:11
@DomFijan DomFijan requested review from joaander, tcmoore3 and pepak13 and removed request for a team April 23, 2024 20:11
Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

It is nice to add generality, but we should to be clear in the documentation that the resulting Box is rotated relative to the input vectors the user provides (when they are not upper triangular).

Namely, the output of to_matrix will not match the same as the input to from_matrix in general.

Otherwise, users might expect to be able to place new particles within the box defined by their original lattice vectors. Is there a way for the users to get the resulting rotation so that they can transform their input coordinates and orientations into the new box frame?

hoomd/pytest/test_box.py Outdated Show resolved Hide resolved
Co-authored-by: Joshua A. Anderson <joaander@umich.edu>
@DomFijan
Copy link
Contributor Author

It is nice to add generality, but we should to be clear in the documentation that the resulting Box is rotated relative to the input vectors the user provides (when they are not upper triangular).

Namely, the output of to_matrix will not match the same as the input to from_matrix in general.

I'll update the docs.

Otherwise, users might expect to be able to place new particles within the box defined by their original lattice vectors. Is there a way for the users to get the resulting rotation so that they can transform their input coordinates and orientations into the new box frame?

This must be doable. How should this rotation be given back to the user? Property of the box?

@joaander
Copy link
Member

This must be doable. How should this rotation be given back to the user? Property of the box?

That is unclear. Ideally, the function could return a tuple (box, rotation), but that would be an API breaking change. Perhaps an example code in the docs that works from the original matrix and the resulting output from_matrix? rowan might have functions that help here.

@janbridley janbridley self-requested a review April 29, 2024 15:11
@janbridley
Copy link
Contributor

After discussing offline: we could name the feature from_basis_vectors. This could return the new box and the rotation matrix as a tuple (as desired), and it maintains the symmetry between from_box and to_box. It does not fix the naming mismatch between Freud and HOOMD though, so it's not a perfect solution.

@DomFijan
Copy link
Contributor Author

pre-commit.ci autofix

@DomFijan
Copy link
Contributor Author

pre-commit.ci autofix

@DomFijan DomFijan changed the title Add support for box.from_matrix for non-triangular matrices Add support for creation of box object from non-triangular matrices Apr 30, 2024
@DomFijan DomFijan requested a review from joaander April 30, 2024 22:32
@DomFijan
Copy link
Contributor Author

DomFijan commented May 3, 2024

pre-commit.ci autofix

@joaander
Copy link
Member

I don't know what happened here, but this branch is deleting files and modifying many files that it should not. I am restoring them now.

@joaander
Copy link
Member

joaander commented May 15, 2024

It seems that there were changes that undid many recent pull request merges:
#1758, #1757, #1756, #1748, #1772 and possibly more. I will squash merge to clean up the history.

For future reference: Merging from a trunk-patch up into a branch that will be merged onto trunk-minor is fine. All those commits will eventually be merged there. The problem is when you merge trunk-minor or trunk-major into a branch that is planned to be merged into trunk-patch or trunk-major into a branch that will be merged into trunk-minor. To fix such an accidental merge, git reset --hard <commit before merge> and git push --force.

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks!

hoomd/box.py Outdated Show resolved Hide resolved
@joaander joaander added the validate Execute long running validation tests on pull requests label May 15, 2024
@janbridley
Copy link
Contributor

janbridley commented May 15, 2024

Tests were failing on my macOS machine due to floating point errors from linalg.solve. Swapping to np.float64 instead of np.float32 fixed this, and brings the default in line with the previous from_matrix function which used double precision by default. I also parametrized some of the new tests to check a few extra values.

@joaander joaander merged commit 68461aa into trunk-minor May 16, 2024
40 checks passed
@joaander joaander deleted the feat/non_triangular_box_matrices_for_box_from_matrix branch May 16, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants