Feature/new box resize #618
Conversation
Just wraps a _hoomd.BoxDim class
min and max must be overloaded for all Python and C++ derived Variants range is automatically implemented. These are necessary for some updaters to properly interpolate values.
Technically this removes the balance (MPI domain decomposition) class; however, this is not working with the current development branch for v3 and a replacement is instituted in a different branch.
Also adds a hoomd.update module folder with __init__.py file
b-butler
requested review from
a team and
joaander
and removed request for
a team
if the C++ side is used they are set to None the setters for the C++ side also check if the variables are set to None before attempting to use the new C++ object for the python variables
|
@Joander that is my fault, I noticed some unintuitive behavior with the logger and just went to fix it without checking tests. I should have just created a PR. |
Co-authored-by: Joshua A. Anderson <joaander@umich.edu>
hoomd/BoxResizeUpdater.cc
Outdated
| } | ||
|
|
||
| auto initial_box = getBoxDimFromPyObject(m_initial_box); | ||
| auto final_box = getBoxDimFromPyObject(m_final_box); |
There was a problem hiding this comment.
In practice, I expect getCurrentBox to be called much more frequently than users requesting the current box in Python, so I would suggest storing BoxDim objects as the members so that you don't have to do this every time and instead making Python boxes if/when the user requests it.
There was a problem hiding this comment.
The problem come in when the user does something like box_resize.box1.L = 10. This should change the internal box. Currently we export the BoxDim class wrapped in a std::unique_ptr. This prevents having multiple full pointers, so we have to copy to store the BoxDim object directly in C++. We could change this to a std::shared_ptr like we do elsewhere, I just don't know if this would break existing code.
There was a problem hiding this comment.
I see, so right now the Python box object is the single source of truth. I'm not sure where you're referring to the unique_ptr, but since the getBoxDimFromPyObject is just pulling the pointer to the _cpp_obj from the Python box, what exactly is the type of _cpp_obj? I don't know the pybind API very well, is it a pointer to the BoxDim object, a copy of the object, or is it wrapped in some intermediate layer? Is it feasible to just return a const BoxDim [*|&] from getBoxDimFromPyObject so that you don't make new objects each time this is called?
Co-authored-by: Vyas Ramasubramani <vramasub@umich.edu>
Co-authored-by: Vyas Ramasubramani <vramasub@umich.edu>
Allows setting boxes in a ducktyped way.
|
I see two unresolved comments, one regarding the |
This lets us not switch context to Python unnecessarily while also ensuring that we can change the box on the Python side.
|
I've discussed my final questions with @b-butler and I'm happy with this PR. @joaander if you wish to discuss whether or not we should allow changing dimensionality in simulations further I'm open to doing that. I don't think it's critical to decide that in this pull request, so I'm going to approve it as is. |
|
I’m removing myself as a reviewer on this PR to cut down on my notifications, it seems well covered by past and current reviews. Thanks! |
Description
An updated Python Box class and an updated C++ and Python BoxResize{Updater} class. Introduces a new module folder hoomd/update and the min, max, and range methods for
Variants as well.The new Box class's interface is modeled in part from freud, and makes extensive use of properties. Unlike the previous Python
boxdimclass, this class always has an up to date C++BoxDimobject.The new BoxResize interface uses
ParameterDictlike previously converted updaters. In addition, the interface uses an initial and final box along with a single variant used for interpolation between the boxes.How Has This Been Tested?
Tested locally in a jupyter notebook.