Fix JSON serialization for Molecule #745
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattwthompson
commented
Oct 26, 2020
Comment on lines
+123
to
+125
| # TODO: More generally check for bytes in dict | ||
| if "conformers" in d.keys(): | ||
| d = _prep_numpy_data_for_json(d) |
There was a problem hiding this comment.
This is only a stopgap solution. It may be better to more generally check for the type of the data, but this field is the only one in the toolkit that currently causes this issue.
|
Passing locally after the version bump |
j-wags
approved these changes
Oct 27, 2020
| @@ -416,6 +417,18 @@ def test_serialization_no_conformers(self): | |||
| pickle_copy = pickle.loads(pickle.dumps(mol)) | |||
| assert mol == pickle_copy | |||
|
|
|||
| def test_json_numpy_roundtrips(self): | |||
There was a problem hiding this comment.
👍 This is a cool test. I've always been vaguely concerned about precision loss and compounding rounding errors, so it's really neat to see this.
There was a problem hiding this comment.
I was pleasantly surprised to see it work
Co-authored-by: Jeff Wagner <jwagnerjpl@gmail.com>
|
LGTM is hanging but the changes here should be small enough in scope to not produce huge warning lights |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #547
This tries to patch the
dictinside ofSerializable.to_json()to convert the conformers from bytes to lists, which surprisingly seem to survive round-trips. This should fixMoleculeserialization but is almost guaranteed to not work for other classes class that need to serialize array data (Topology.box_vectors?). Improvements to the_prep_numpy_data_for_jsonfunction should enable this, it's currently a half-baked attempt at the general solution.