Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 class method for Box class which create a box from Lx,Ly,Lz and angles #1234
Add class method for Box class which create a box from Lx,Ly,Lz and angles #1234
Changes from 19 commits
0d6f975
0717da1
ae95e63
5a388bf
58a56f6
746b923
2f9adcf
c7ad750
c399ff3
2a99532
16d028a
dc74488
7706c2f
8860961
12b4a1a
0e78e47
da9d7ea
aa4354a
d536985
461485e
90dd30c
8e27b3c
863ac23
5bd2c08
6a9b740
1a45db1
ace7fac
6f6932e
ae96836
3aab7cb
a7c18e2
c26e88e
ae56eff
a3daf1b
dffb772
a3548a1
a3449d8
b32ed14
ca710c3
ad8d8b3
b51d8d7
d78e08e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How is this different than
from_matrix
?Box.from_matrix
makes a box from a matrix where the columns are the lattice vectors. This seems very similar to that.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.
Not sure how this got past me but with this in mind this opens a whole new debate.
from_matrix
in freud basically uses lattice vectors as input, but this is not stated explicitly. Instead a link to hoomd docs is given which further complicates the problem. In hoomdfrom_matrix
wants upper triangular matrix that is already a box like object as defined in hoomd docs. What we should do is to have freudsfrom_matrix
take upper triangular matrix as input, such that its functionality is equivalent to hoomd's and addfrom_lattice_vectors
which has same function as currentfrom_matrix
?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.
I think it would be best to add to freud's documentation and explicitly state that the columns of the input to
from_matrix
are the lattice vectors of the box. Any other improvements to the documentation offrom_matrix
to make it less confusing would also be welcome.Looking at the diff of the code between
from_lattice_vectors
andfrom_matrix
, they do the exact same thing so I don't see a need to have both factory methods hanging around.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.
Is the functionality of freud's
from_matrix
equivalent to hoomd'sfrom_matrix
?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 appears that hoomd's
from_matrix
requires an upper triangular matrix, whereas freud's is more general and does not.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.
I think a square here better conveys the intent, as below we have a very similar function with two different variables
(a2x * a3x)
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.
This line appears to take the square root of a negative number at times, resulting in NaNs.
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.
some combinations of angles are not possible-there is likely a constraint on their sum or something like that. Is this taken into account?
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.
Not in how I wrote the test case, which would explain the failures. The code should throw an error if the user gives inputs which violate the constraints.
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.
from_lattice_vectors was removed
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.
Since this method (and
from_box_lengths_and_angles
) just replaces two lines of user script, I don't think it's needed. I am open to discussion though.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's just a convenience function. I love these. If you strongly feel they are not needed we can remove them. Up to you.
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.
My personal logic is that if we have a convenience function for making
UnitCell
's in thedata
module forBox.from_lattice_vectors
andBox.from_box_lengths_and_angles
, then we should also have a convenience function for every factory method in theBox
class (likecube
,from_box
,from_matrix
,square
, etc.). There are quite a few of those and it may get cumbersome to have so many convenience functions hanging around.If you like having that convenience function, add one to your python scripts. It only takes two lines to write it, after all :)
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.
While I see what you're saying, there are a significant number of users/use cases for this specific functionality: making the translation between freud boxes and data from crystallography files more transparent is a good thing in my opinion. I think adding this particular convenience function provides enough to users that we can probably do without the rest mentioned above. If this truly doesn't fit though, I can live without it.
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.
What data does a crystallography file typically contain? If its a common use case to make a freud
UnitCell
from crystallography file data, then it makes sense to have a convenience function for it. I personally don't work a lot with those, so I'm not entirely sure what would be most helpful for others who work with it.I think this topic is worth further offline discussion as well.
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.
CIF files (the most common file type, at least for computational/experimental researchers) have unit cell information encoded as a/b/c/α/β/γ. For DFT peeps, VASP and ABINIT provide lattice vectors so those would work without this function.
Crystallography Open Database
AFlow:
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.
I think a great test to add would be to start with 3 random lengths and 3 random angles, then call
from_box_lengths_and_angles
to get a box, then callto_box_lengths_and_angles
on that box and assert that you recover the original random lengths and angles.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.
Each of the six numbers should be the output of a different call to
np.random.rand
. That way, each time the test is run it gets run with a different random value.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.
I am currently working on adding this.