-
Notifications
You must be signed in to change notification settings - Fork 10
Add precision argument to hoomd template, allowing for float64 data type to be used. #495
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
base: trunk-minor
Are you sure you want to change the base?
Conversation
…optional values from hoomd for single and double precision
|
Initial notes here: Quick question. Should we increment the hoomd schema version following this change? It should be completely backward compatible, but potentially some unexpected floating point math could occur if you created a file with this PR, but then read it in a previous version of GSD, then finally write back to disk, any float64 values at this point would be converted to |
joaander
left a comment
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! I have a few comments.
Not sure we need every float to take the "double" precision though, unless there are strong thoughts on this.
If a user sets a global precision="double", they would be surprised if some of the content was written in single precision.
An alternate API would be to remove the global setting and keep the same precision that the user provides for every field, giving them fine-grained control.
Should we increment the hoomd schema version following this change?
Yes, bump the version to 2.0. Update hoomd-schema.rst to note that data types may be float32 or float64. This is a breaking change because a reader following the 1.x specification will specifically expect float32 data and fail to load float64.
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 would prefer to leave fl.pyx unmodified. write_chunk already has a mechanism to choose the precision --- the data type of the numpy array. read_chunk callers should expect a numpy array that matches the data type in the file. Those callers can cast it should they choose to do so.
You can achieve your intended "convert on write" with logic in HOOMDTrajectory::append.
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 leave it up to you whether you want to lazily cast or cast on validate.
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's preferred to lazily cast. An important use case would be to: write out frames in a trajectory in 32 bit precision, then write out a final restart Frame in 64 bit precision. However, we lose the precision if we cast on validate when we write out the trajectory as we go. See test_write_multiple_precision as an example of writing the same Frame with different precisions.
At commit df776ed, we now expectedly cause these tests to fail, since the data objects are modified to represent the correct float_type.
FAILED gsd/test/test_hoomd.py::test_fallback[(r,w)] - AssertionError:
FAILED gsd/test/test_hoomd.py::test_fallback[(a,x)] - AssertionError:
FAILED gsd/test/test_hoomd.py::test_fallback[(r,a)] - AssertionError:
FAILED gsd/test/test_hoomd.py::test_fallback_to_frame0[(r,w)] - AssertionError:
FAILED gsd/test/test_hoomd.py::test_fallback_to_frame0[(a,x)] - AssertionError:
FAILED gsd/test/test_hoomd.py::test_fallback_to_frame0[(r,a)] - AssertionError:
Should I reopen the PR to be pulled against trunk-major then? |
My hesitancy to this would be if a user uses numpy or list of floats, the default precision would be 64, which then would get written into their gsd object as such without manually changing to float32. That's a change in default API and users may not realize that their file sizes would be twice as big, which is why I lean towards specifying double-precision somewhere in the API. |
Co-authored-by: Joshua A. Anderson <joaander@umich.edu>
No, the PR is fine. I plan to collapse to a single trunk branch after the next release. Development has slowed to the point that there is no longer a need for separate trunk branches.
I understand, but this will be a breaking release. Ignore backwards compatibility for the moment. Ask yourself what a new user (one with no prior knowledge and has not read the documentation) would expect. Would they assume that I don't have a strong preference either way on global vs. granular. I'm fine with a global flag. But if we choose to offer granular control, than I would prefer that the user-provided precision is maintained. |
…n write, while keeping python Frame object untouched
Description
Option to specify double precision float64 dtype for hoomd schema.
Motivation and Context
It is required to write complete restart files in the HOOMD ecosystem, as floats such as charges have to round precisely to 0, and can deviate at really small float values when converting from float64 to float32 and back again, exacerbated by large enough system sizes.
Resolves glotzerlab/hoomd-blue#2194
How Has This Been Tested?
Test reading and writing both single and double precision hoomd files, checking that the dtype is kept consistent.
Checklist:
doc/credits.rst) in the pull request source branch.CHANGELOG.rst.