Added custom box passing when saving #330

justinGilmer · 2017-03-10T04:14:03Z

Modified to_parmed() function to accept custom box sizes.

Modified saver() to accept custom box sizes

Modified test_compound to accept custom boxes.

I need to add more tests to ensure that the data is properly saved and converted to Angstroms.

Made box a required argument for to_parmed().

Modified `to_parmed()` function to accept custom box sizes. Modified `saver()` to accept custom box sizes Modified test_compound to accept custom boxes.

ctk3b · 2017-03-10T14:55:32Z

Looks good, thanks! Will merge once you've got some appropriate unit tests in there to make sure the info makes it all the way through to parmed and the saved files.

Ran into a few limitations when trying to save/load for my unit tests. 1. Cannot load `.lammps` data files. - Seems not an issue, contains a lot of information not really needed 2. HOOMDXML unit conversion on loading - Writing out `.hoomdxml` files converts box parameters to Angstroms - when loading back in, units stay in Angstroms - Unsure how to correct, since we are delegating the loading to mdtraj 2.1 HOOMDXML box custom issues - possibly incorrectly moving to origin? - just a padded box has correct lengths, but angstroms - custom box has wildly incorrect lengths - unsure what is causing it - custom box parameters are affecting bounding box upon loading - still a mystery at the moment 3. Hard to test every file that parmed and mbuild can save to - Not every file type we write out can be loaded back in: (`.lammps`, `.gsd`, etc.) - Unsure how to check using tests within mBuild to ensure data we write out is correct - havent really tested using unit tests if the data we write out in the mBuild supported savers are correct (at least in the tests region). 4. Box seems pretty rudimentary - Possible issue to convert to a triclinic system? - lots of assumptions about the boxes in the savers (angles = 90, etc.) - might be a pretty hefty overhaul of the `Box` class 5. Units conversions everywhere - no real method to the madness in terms of when the units are converted from nm to Angstroms - Since `save()` is the way to output files, all unit conversions there? - Assuming using save is the only way to output files - would make sense to enforce this? Things to Note 1. Lots of print statements in here now - Still very much a WIP - want to get some feedback and discussion 2. WIP - I have my tests failing on purpose at the moment to observe outputs. 3. For `parmed` supported structures and `mdtraj` supported loadable files - Seem to work properly - unsure even angstrom to nm conversion is correct? - maybe `.hoomdxml` just a fluke in the loading using `mdtraj`?

justinGilmer · 2017-03-14T23:05:12Z

Since commit message is a bit hard to read, posting it here, since it will also be useful for the questions I have.

Ran into a few limitations when trying to save/load for my unit tests.

Cannot load .lammps data files.
- Seems not an issue, contains a lot of information not really needed
HOOMDXML unit conversion on loading
- Writing out .hoomdxml files converts box parameters to Angstroms
- when loading back in, units stay in Angstroms
- Unsure how to correct, since we are delegating the loading to mdtraj

2.1 HOOMDXML box custom issues
- possibly incorrectly moving to origin?
- just a padded box has correct lengths, but angstroms
- custom box has wildly incorrect lengths
- unsure what is causing it
- custom box parameters are affecting bounding box upon loading
- still a mystery at the moment

Hard to test every file that parmed and mbuild can save to
- Not every file type we write out can be loaded back in: (.lammps, .gsd, etc.)
- Unsure how to check using tests within mBuild to ensure data we write out is correct
- havent really tested using unit tests if the data we write out in the mBuild supported savers are correct (at least in the tests region).
Box seems pretty rudimentary
- Possible issue to convert to a triclinic system?
- lots of assumptions about the boxes in the savers (angles = 90, etc.)
- might be a pretty hefty overhaul of the Box class
Units conversions everywhere
- no real method to the madness in terms of when the units are converted from nm to Angstroms
- Since save() is the way to output files, all unit conversions there?
- Assuming using save is the only way to output files
  - would make sense to enforce this?

Things to Note

Lots of print statements in here now
- Still very much a WIP
- want to get some feedback and discussion
WIP
- I have my tests failing on purpose at the moment to observe outputs.
For parmed supported structures and mdtraj supported loadable files
- Seem to work properly
- unsure even angstrom to nm conversion is correct?
- maybe .hoomdxml just a fluke in the loading using mdtraj?

Removed wrapping issue from hoomdxml. Bounding box preserves proper length of box based on particle positions. Custom box support to allow smaller or larger boxes to write to data files.

Had extraneous arguments in the load function. **kwargs handles this.

ctk3b · 2017-03-18T16:02:07Z

Can you resolve the merge conflicts here? Need to get this fix in sooner rather than later.

ctk3b · 2017-03-22T16:06:40Z

@justinGilmer will you have time to fix this up today?

Error where box was not being properly set, causing DIV0 errors. Maxs and mins of `Box` class was not being set correctly, not updating lengths, which was essential for `to_parmed()` to function properly.

justinGilmer · 2017-03-22T19:11:08Z

I think all the issues have been ironed out. Just waiting for the CI's to finish.

ctk3b · 2017-03-22T19:29:20Z

mbuild/compound.py

@@ -1304,16 +1304,18 @@ def save(self, filename, show_ports=False, forcefield_name=None,
        # pad non periodic faces with .25nm buffers
        if box is None:
            box = self.boundingbox
+            box_vec_max = box.maxs.tolist()
+            box_vec_min = box.maxs.tolist()


This should definitely be box.mins but could you explain why the list cast is necessary here?

The issue was when we were just changing these values, the setter was not being called it seemed. box.lengths was not being updated. At least with the 2 single residue test case. The best way I could see would be to just unwrap that numpy array and perform the operations on it. Then pass it back as one numpy array which seemed to update box.lengths as well.

EDIT:
TLDR, it seemed that changing in place the numpy array values would not force the box setter to update the lengths. By converting to a list so it could be operated on and then passing that back in as a numpy array, that seemed to alleviate this issue.

I think it was just that the residue case was very different. It had no bounding box essentially. It was a [0,0,0] box. Which seemed to affect the setters.

EDIT:
I see what you mean about the two max calls. Fixing now.

Ah that makes sense. The setters don't work for elements of the array getting changed. Probably not worth tackling until the box rewrite.

box mins list was assigned the maxs of the box.

ctk3b

Looks great, thanks! Just some final tidying up and it's good to go.

ctk3b · 2017-03-22T20:09:10Z

mbuild/tests/test_compound.py

+            for attr in box_attributes:
+                pad_attr = getattr(padded_ch3.boundingbox, attr)
+                custom_attr = getattr(custom_ch3.boundingbox, attr)
+                assert(np.array_equal(pad_attr, custom_attr))


Assert is a statement so parentheses should be removed.

ctk3b · 2017-03-22T20:11:12Z

mbuild/compound.py

+    **kwargs : keyword arguments
+        Arbitrary keyword arguments.
+
+    Args:


These args belong in the save function where I believe they already are.

However, load is it's own separate function, outside of Compound. Since save and its **kwargs are passed to other savers and such, would it be reasonable to have both defined?

Are you asking if both *args and **kwargs should be defined here? *args gets a bit tricky because it cares about order. **kwargs could have a bit more description like "arguments passed down to mdtraj".

That being said, my original comment was about the content of the Args: section that I tagged. ref_distance/mass/energy are not arguments relevant to any of the loaders. They only apply to the hoomd saver so they shouldnt be in the docstring for the load function.

Ah gotcha, that makes sense. And youre correct, I was recalling the source code for mdTraj incorrectly. Just looked it up and I understand it now.

ctk3b · 2017-03-22T20:11:53Z

mbuild/tests/test_compound.py

@@ -393,10 +407,10 @@ def test_intermol_conversion2(self, ethane, h2o):
    def test_parmed_conversion(self, ethane, h2o):
        compound = mb.Compound([ethane, h2o])

-        structure = compound.to_parmed()
+        structure = compound.to_parmed(box=compound.boundingbox)


The default right now will be the boundingbox plus a buffer is nothing specified, correct?

That is if they call save(). If they just call to_parmed, that is not currently happening.

Unsure how we would want to handle this actually. Since we were wanting the padding to be handled by the saver. I guess I could check if the box passed in is None, if so, pad and move on. That would handle use cases when just calling this function. Since save() does the padding before passing that box to to_parmed()

Incorrect description of arguments that could be passed to load function.

ctk3b · 2017-03-23T14:44:32Z

Almost done! Just gotta remove the parentheses around that assert statement.

to_parmed now supports generation of padded boxes without calling save(). Can also pass in custom box as well. If no box passed in, will default to a .25nm buffer on all sides of box.

ctk3b · 2017-03-23T17:14:48Z

mbuild/compound.py

+                    box_vec_max[dim] += 0.25
+                    box_vec_min[dim] -= 0.25
+            box.mins = np.asarray(box_vec_min)
+            box.maxs = np.asarray(box_vec_max)


This code is duplicated now. I think you can re-organize this so it only exists in to_parmed

1. Modified to_parmed() to handle the box padding. 2. save() no longer handles box padding 3. Modified the 3 mbuild data file writers (hoomd, lammps, gsd) to also use the parmed structure for box information.

Changed mBuild file writers to instantiate the box in 1 line.

ctk3b · 2017-03-23T20:14:25Z

mbuild/formats/hoomdxml.py

    forcefield = True
    if structure[0].type == '':
        forcefield = False
    xyz = np.array([[atom.xx, atom.xy, atom.xz] for atom in structure.atoms])

-    box.lengths = np.array([structure.box[0], structure.box[1], structure.box[2]])
-    box.maxs *= 10.


Did you mean to delete this?

Deleted angstroms conversion of the box maxs member.

Looped over 3 dimensions instead of enumerating the periodicity.

Added custom box passing when saving

Added custom box passing when saving

c967e7b

Modified `to_parmed()` function to accept custom box sizes. Modified `saver()` to accept custom box sizes Modified test_compound to accept custom boxes.

justinGilmer added 2 commits March 17, 2017 13:32

Remove wrapping issue from hoomdxml.py

7d248ee

Removed wrapping issue from hoomdxml. Bounding box preserves proper length of box based on particle positions. Custom box support to allow smaller or larger boxes to write to data files.

Removed extraneous optional args

dcb9ab6

Had extraneous arguments in the load function. **kwargs handles this.

Merge branch 'master' into boxSaver

64c0667

Fixed save_resnames_single

71475d7

Error where box was not being properly set, causing DIV0 errors. Maxs and mins of `Box` class was not being set correctly, not updating lengths, which was essential for `to_parmed()` to function properly.

ctk3b reviewed Mar 22, 2017

View reviewed changes

Fixed incorrect variable assignment

f97cd5c

box mins list was assigned the maxs of the box.

ctk3b requested changes Mar 22, 2017

View reviewed changes

Removed improper **kwarg docstring

8e78cce

Incorrect description of arguments that could be passed to load function.

ctk3b mentioned this pull request Mar 23, 2017

Applying forcefield to mBuild compound mosdef-hub/foyer#105

Closed

justinGilmer added 2 commits March 23, 2017 11:21

Removed parentheses from assert statement

56fcb9f

to_parmed padded box changes

f87b499

to_parmed now supports generation of padded boxes without calling save(). Can also pass in custom box as well. If no box passed in, will default to a .25nm buffer on all sides of box.

ctk3b reviewed Mar 23, 2017

View reviewed changes

justinGilmer added 2 commits March 23, 2017 14:32

Modified mbuild savers and to_parmed for box padding

5f61f9f

1. Modified to_parmed() to handle the box padding. 2. save() no longer handles box padding 3. Modified the 3 mbuild data file writers (hoomd, lammps, gsd) to also use the parmed structure for box information.

Modification of Box instantiation

cd49c24

Changed mBuild file writers to instantiate the box in 1 line.

ctk3b reviewed Mar 23, 2017

View reviewed changes

justinGilmer added 2 commits March 23, 2017 15:16

Overzealous deletion

ae201c1

Deleted angstroms conversion of the box maxs member.

Simplified for angstrom conversion loop in to_parmed

c796717

Looped over 3 dimensions instead of enumerating the periodicity.

ctk3b merged commit 5012262 into mosdef-hub:master Mar 23, 2017

justinGilmer deleted the boxSaver branch May 18, 2017 16:05

summeraz mentioned this pull request Aug 21, 2018

Fix/hoomd gsd xml #452

Merged

umesh-timalsina referenced this pull request in GOMC-WSU/MoSDeF-GOMC Mar 22, 2022

Merge pull request #330 from justinGilmer/boxSaver

3937748

Added custom box passing when saving

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added custom box passing when saving #330

Added custom box passing when saving #330

justinGilmer commented Mar 10, 2017

ctk3b commented Mar 10, 2017

justinGilmer commented Mar 14, 2017

ctk3b commented Mar 18, 2017

ctk3b commented Mar 22, 2017

justinGilmer commented Mar 22, 2017

ctk3b Mar 22, 2017 •

edited

Loading

justinGilmer Mar 22, 2017 •

edited

Loading

justinGilmer Mar 22, 2017 •

edited

Loading

ctk3b Mar 22, 2017

ctk3b left a comment

ctk3b Mar 22, 2017

ctk3b Mar 22, 2017

justinGilmer Mar 22, 2017

ctk3b Mar 22, 2017

justinGilmer Mar 22, 2017

ctk3b Mar 22, 2017

justinGilmer Mar 23, 2017

ctk3b commented Mar 23, 2017

ctk3b Mar 23, 2017

ctk3b Mar 23, 2017

Added custom box passing when saving #330

Added custom box passing when saving #330

Conversation

justinGilmer commented Mar 10, 2017

ctk3b commented Mar 10, 2017

justinGilmer commented Mar 14, 2017

ctk3b commented Mar 18, 2017

ctk3b commented Mar 22, 2017

justinGilmer commented Mar 22, 2017

ctk3b Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

justinGilmer Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

justinGilmer Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctk3b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctk3b commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctk3b Mar 22, 2017 •

edited

Loading

justinGilmer Mar 22, 2017 •

edited

Loading

justinGilmer Mar 22, 2017 •

edited

Loading