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
Add support for creation of box object from non-triangular matrices #1769
Conversation
There was a problem hiding this 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?
Co-authored-by: Joshua A. Anderson <joaander@umich.edu>
I'll update the docs.
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 |
After discussing offline: we could name the feature |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
pre-commit.ci autofix |
pre-commit.ci autofix |
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. |
It seems that there were changes that undid many recent pull request merges: For future reference: Merging from a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Tests were failing on my macOS machine due to floating point errors from |
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
Checklist:
sphinx-doc/credits.rst
) in the pull request source branch.