Add class method for Box class which create a box from Lx,Ly,Lz and angles #1234

DomFijan · 2024-03-07T17:22:24Z

Implements classmethods that create a box class from ~~lattice vector matrix and~~ L1, L2, L3 and angles, as well as methods that give L1, L2, L3 and angles from box class.

Description

Crystals are usually defined via their lattice vectors and wykoff sites or lengths of lattice vectors and angles; while freud and hoomd use cell parameter matrix. In this PR I implement class method for creation of box ~~from lattice vectors or~~ from lattice vector lengths and angles. I've also implemented a method that gives lengths of box vectors and angles from box object.

Motivation and Context

Lattice vectors or lattice vector lengths and angles are used to define real crystals and having a method that works with this makes creating crystals easier.

How Has This Been Tested?

Tests were added for new methods for box testing.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds or improves functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
I have updated the documentation (if relevant).
I have added tests that cover my changes (if relevant).
All new and existing tests passed.
I have updated the credits.
I have updated the Changelog.

for more information, see https://pre-commit.ci

…t cell class

…tzerlab/freud into feat_from_to_lattice_vectors

for more information, see https://pre-commit.ci

…tzerlab/freud into feat_from_to_lattice_vectors

for more information, see https://pre-commit.ci

janbridley · 2024-03-20T14:44:01Z

I personally am a proponent of using math.sqrt and math.acos, as for single values it is significantly faster than the numpy alternatives. The difference is on the order of μs, but free performance is free imo.

DomFijan · 2024-03-20T14:57:50Z

I personally am a proponent of using math.sqrt and math.acos, as for single values it is significantly faster than the numpy alternatives. The difference is on the order of μs, but free performance is free imo.

It seems numpy sqrt is used throughout freud from what I can tell. If we were to start using math functions we should change all occurrences. I agree it makes no sense to leave performance on the table, but realistically the differences will be so small they are inconsequential. Perhaps this would be a good first issue to tackle for someone new?

janbridley · 2024-03-20T15:03:02Z

freud/box.pyx

+        v2 = lattice_matrix[2]
+        Lx = np.sqrt(np.dot(v0, v0))
+        a2x = np.dot(v0, v1) / Lx
+        Ly = np.sqrt(np.dot(v1, v1) - a2x * a2x)


Suggested change

Ly = np.sqrt(np.dot(v1, v1) - a2x * a2x)

Ly = np.sqrt(np.dot(v1, v1) - a2x**2)

I think a square here better conveys the intent, as below we have a very similar function with two different variables (a2x * a3x)

tests/test_box_Box.py

janbridley · 2024-03-20T15:41:13Z

I personally am a proponent of using math.sqrt and math.acos, as for single values it is significantly faster than the numpy alternatives. The difference is on the order of μs, but free performance is free imo.

It seems numpy sqrt is used throughout freud from what I can tell. If we were to start using math functions we should change all occurrences. I agree it makes no sense to leave performance on the table, but realistically the differences will be so small they are inconsequential. Perhaps this would be a good first issue to tackle for someone new?

Noted, and sounds good! I will raise an issue.

Edit: See #1236

Co-authored-by: Tommy Waltmann <[email protected]>

freud/box.pyx

tommy-waltmann

One final thing, then we merge.

Great work on this, I think many people will find this useful!

tommy-waltmann · 2024-03-22T13:38:54Z

tests/test_box_Box.py

+            1,
+            2,
+            3,
+            np.pi / 3,
+            np.pi / 2.25,
+            np.pi / 2.35,


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.

I am currently working on adding this.

for more information, see https://pre-commit.ci

tommy-waltmann · 2024-03-22T13:57:22Z

freud/box.pyx

+        a2 = np.array([L2 * np.cos(gamma), L2 * np.sin(gamma), 0])
+        a3x = np.cos(beta)
+        a3y = (np.cos(alpha) - np.cos(beta) * np.cos(gamma)) / np.sin(gamma)
+        a3z = np.sqrt(1 - a3x**2 - a3y**2)


This line appears to take the square root of a negative number at times, resulting in NaNs.

some combinations of angles are not possible-there is likely a constraint on their sum or something like that. Is this taken into account?

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.

ispivack · 2024-03-26T17:19:46Z

All looks good to me, just one comment. If we want to just use freud for the EBT box, it would be good to have a convenience function converting to a unit vector + lengths description. Do we feel that this would fit into freud, or should this just be separately implemented in ebonds?

tommy-waltmann · 2024-03-26T17:32:28Z

All looks good to me, just one comment. If we want to just use freud for the EBT box, it would be good to have a convenience function converting to a unit vector + lengths description. Do we feel that this would fit into freud, or should this just be separately implemented in ebonds?

I would start by adding the functionality into ebonds. If it becomes cumbersome to have to do the conversion in ebonds constantly, then adding a method to freud to do that might be worth it.

Right now, the best way to get lengths and unit vectors would be to get the box vectors (they are the columns of box.to_matrix()) and get magnitude and direction from that.

janbridley · 2024-03-26T18:30:48Z

np.testing.assert_allclose sets atol=0 by default. This will not work with floating point calculations, so I set atol to 1e-14. However, the tests are still failing due to mismatched NaNs.

tommy-waltmann · 2024-03-26T21:51:18Z

np.testing.assert_allclose sets atol=0 by default. This will not work with floating point calculations, so I set atol to 1e-14. However, the tests are still failing due to mismatched NaNs.

It's fine to leave atol as 0.0. According to the documentation for assert_allclose: "It compares the difference between actual and desired to atol + rtol * abs(desired)", so having an rtol is enough to account for floating point error.

joaander · 2024-04-01T10:24:32Z

np.testing.assert_allclose sets atol=0 by default. This will not work with floating point calculations, so I set atol to 1e-14. However, the tests are still failing due to mismatched NaNs.

It's fine to leave atol as 0.0. According to the documentation for assert_allclose: "It compares the difference between actual and desired to atol + rtol * abs(desired)", so having an rtol is enough to account for floating point error.

A non-zero atol is needed when a value close to zero should be accepted as a desired (exactly) zero value. atol + rtol * abs(desired) == 0.0 when atol=0 and desired=0.

janbridley · 2024-04-11T20:37:47Z

I have tested this method against box lengths and angles from 1,104 real CIF files and all cases pass. I can export the box params as a numpy array and save it as test data, or we can adjust the current test to be more realistic.

tommy-waltmann · 2024-04-11T21:28:32Z

I have tested this method against box lengths and angles from 1,104 real CIF files and all cases pass. I can export the box params as a numpy array and save it as test data, or we can adjust the current test to be more realistic.

The code should be written so invalid sets of angle inputs raise an error to the user instead of creating invalid boxes. This PR needs an update to the code, not just the tests, before it's ready to be merged.

Ideally, we would know the mathematical constraints on the box angles so we could check that the inputs are valid, and we should make an effort to figure out what those are. If we cannot get those relations, then we could move forward with an updated set of tests and a "garbage in, garbage out" note in the documentation, but again, I only think we should take the second option if we have already given the first option some effort and failed.

…for angles and raise valueerror when violated

DomFijan · 2024-04-11T23:14:41Z

I think I figured it out. The restriction is the following:
$1+2\cos(\alpha)\cos(\beta)\cos(\gamma) - \cos(\alpha)^2 - \cos(\beta)^2 - \cos(\gamma)^2>0$
This is part of the computation of volume of a Parallelepiped. This guarantees that the volume of the box defined is positive and must always be true. This statement should be equivalent to requiring the value under the sqrt in the from_box_lengths_and_angles function be positive (haven't done the maths to confirm that).

The geometric interpretation of this is the violation of the cosine law.

tommy-waltmann

Great work! I know this will be useful for lots of peeps using freud.

DomFijan and others added 19 commits February 26, 2024 19:36

add from lattice_vectors, and to and from lenghts and angles

0d6f975

Add methods to convert box dimensions and angles

0717da1

Merge branch 'main' into feat_from_to_lattice_vectors

ae95e63

[pre-commit.ci] auto fixes from pre-commit.com hooks

5a388bf

for more information, see https://pre-commit.ci

add type hints and a class method from box lengths and angles for uni…

58a56f6

…t cell class

add type hints

746b923

Merge branch 'feat_from_to_lattice_vectors' of https://github.com/glo…

2f9adcf

…tzerlab/freud into feat_from_to_lattice_vectors

[pre-commit.ci] auto fixes from pre-commit.com hooks

c7ad750

for more information, see https://pre-commit.ci

fix format

c399ff3

more formatting fixes

2a99532

fixes to logic and docs to all new functions

16d028a

add tests

dc74488

[pre-commit.ci] auto fixes from pre-commit.com hooks

7706c2f

for more information, see https://pre-commit.ci

update some docstrings for consistency

8860961

Merge branch 'feat_from_to_lattice_vectors' of https://github.com/glo…

12b4a1a

…tzerlab/freud into feat_from_to_lattice_vectors

[pre-commit.ci] auto fixes from pre-commit.com hooks

0e78e47

for more information, see https://pre-commit.ci

linter fix

da9d7ea

added changelog and credits

aa4354a

[pre-commit.ci] auto fixes from pre-commit.com hooks

d536985

for more information, see https://pre-commit.ci

DomFijan requested review from ispivack, janbridley and tommy-waltmann March 20, 2024 13:55

DomFijan marked this pull request as ready for review March 20, 2024 13:56

DomFijan requested a review from a team as a code owner March 20, 2024 13:56

janbridley reviewed Mar 20, 2024

View reviewed changes

Test dimension detection for from_lattice_vectors

461485e

janbridley reviewed Mar 20, 2024

View reviewed changes

tests/test_box_Box.py Outdated Show resolved Hide resolved

tommy-waltmann and others added 4 commits March 21, 2024 16:37

Update freud/box.pyx

a7c18e2

Apply suggestions from code review

c26e88e

Update ChangeLog.md

ae56eff

Co-authored-by: Tommy Waltmann <[email protected]>

update docs to state range of angles allowed

a3daf1b

janbridley reviewed Mar 22, 2024

View reviewed changes

freud/box.pyx Show resolved Hide resolved

janbridley approved these changes Mar 22, 2024

View reviewed changes

tommy-waltmann requested changes Mar 22, 2024

View reviewed changes

tommy-waltmann and others added 2 commits March 22, 2024 09:56

update test

dffb772

[pre-commit.ci] auto fixes from pre-commit.com hooks

a3548a1

for more information, see https://pre-commit.ci

tommy-waltmann reviewed Mar 22, 2024

View reviewed changes

ispivack approved these changes Mar 26, 2024

View reviewed changes

Set nonzero atol for assert_allclose in test_box_Box.py

a3449d8

Merge branch 'main' into feat_from_to_lattice_vectors

b32ed14

update from_box_lengths_and_angles docstring to contain restrictions …

ca710c3

…for angles and raise valueerror when violated

DomFijan added 3 commits April 11, 2024 19:16

raise error if box has negative volume

ad8d8b3

fix sqrt

b51d8d7

fix test to check for imposible boxes

d78e08e

DomFijan requested a review from tommy-waltmann April 23, 2024 16:24

tommy-waltmann approved these changes Apr 25, 2024

View reviewed changes

tommy-waltmann merged commit 8353049 into main Apr 25, 2024
15 checks passed

tommy-waltmann deleted the feat_from_to_lattice_vectors branch April 25, 2024 16:19

tommy-waltmann added this to the v3.1.0 milestone Jun 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

DomFijan commented Mar 7, 2024 •

edited

Loading

janbridley commented Mar 20, 2024

DomFijan commented Mar 20, 2024

janbridley Mar 20, 2024

janbridley commented Mar 20, 2024 •

edited

Loading

tommy-waltmann left a comment

tommy-waltmann Mar 22, 2024

tommy-waltmann Mar 22, 2024

tommy-waltmann Mar 22, 2024

DomFijan Mar 22, 2024

tommy-waltmann Mar 22, 2024

ispivack commented Mar 26, 2024

tommy-waltmann commented Mar 26, 2024

janbridley commented Mar 26, 2024

tommy-waltmann commented Mar 26, 2024

joaander commented Apr 1, 2024

janbridley commented Apr 11, 2024

tommy-waltmann commented Apr 11, 2024

DomFijan commented Apr 11, 2024 •

edited

Loading

tommy-waltmann left a comment

	Ly = np.sqrt(np.dot(v1, v1) - a2x * a2x)
	Ly = np.sqrt(np.dot(v1, v1) - a2x**2)

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

Conversation

DomFijan commented Mar 7, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

janbridley commented Mar 20, 2024

DomFijan commented Mar 20, 2024

janbridley Mar 20, 2024

Choose a reason for hiding this comment

janbridley commented Mar 20, 2024 • edited Loading

tommy-waltmann left a comment

Choose a reason for hiding this comment

tommy-waltmann Mar 22, 2024

Choose a reason for hiding this comment

tommy-waltmann Mar 22, 2024

Choose a reason for hiding this comment

tommy-waltmann Mar 22, 2024

Choose a reason for hiding this comment

DomFijan Mar 22, 2024

Choose a reason for hiding this comment

tommy-waltmann Mar 22, 2024

Choose a reason for hiding this comment

ispivack commented Mar 26, 2024

tommy-waltmann commented Mar 26, 2024

janbridley commented Mar 26, 2024

tommy-waltmann commented Mar 26, 2024

joaander commented Apr 1, 2024

janbridley commented Apr 11, 2024

tommy-waltmann commented Apr 11, 2024

DomFijan commented Apr 11, 2024 • edited Loading

tommy-waltmann left a comment

Choose a reason for hiding this comment

DomFijan commented Mar 7, 2024 •

edited

Loading

janbridley commented Mar 20, 2024 •

edited

Loading

DomFijan commented Apr 11, 2024 •

edited

Loading