Skip to content
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

Add vector-valued variants #1685

Merged
merged 40 commits into from Feb 28, 2024
Merged

Add vector-valued variants #1685

merged 40 commits into from Feb 28, 2024

Conversation

tcmoore3
Copy link
Member

@tcmoore3 tcmoore3 commented Dec 19, 2023

Description

This PR implements a VectorVariant that returns array values as a function of time step, in contrast to the Variant classes that return scalar values as a function of the time step. VectorVariant is templated on the length of the array to take advantage of C++'s strong typing. This PR also implements a VectorVariantBox and a handful of subclasses of VectorVariantBox that return length-6 arrays for use with functions that take box arguments.

The new VectorVariant* C++ classes are implemented in VectorVariant.[h,cc]. In Python, I created a new variant package. The existing (scalar-valued) Variants are defined in the package's __init__.py file, and the box vector variant python classes are in hoomd/variant/box.py.

Motivation and context

This functionality is a first step towards fixing several issues, #1640 and #1641. It will also allow us to replace the arguments box1, box2, and variant with a single hoomd.variant.box.* argument to make the use of update.BoxResize more intuitive.

How has this been tested?

Existing unit tests pass, and my ad-hoc tests of the new box vector variant classes are passing. I still need to add proper unit tests for the vector variants.

Change log

- Add `variant.box` classes that describe boxes that change with time step.

Checklist:

Adds a VectorVariant class.
Adds VectorVariantBox as a subclass of VectorVariant.
Implements VectorVariantBoxConstant and VectorVariantBoxLinear.
Moves variant from a module to a package.

The code compiles and I am able to get the expected boxes as a function
of time for both VectorVariantBoxConstant and VectorVariantBoxLinear
objects.

More to come before I open a PR.
@tcmoore3 tcmoore3 requested review from a team as code owners December 19, 2023 17:54
@tcmoore3 tcmoore3 requested review from joaander, SchoeniPhlippsn and melodyyzh and removed request for a team December 19, 2023 17:54
Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Here are some suggestions to improve this.

As you already noted, we also need to add unit tests.

hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/__init__.py Outdated Show resolved Hide resolved
hoomd/update/box_resize.py Outdated Show resolved Hide resolved
hoomd/variant/__init__.py Outdated Show resolved Hide resolved
sphinx-doc/module-hoomd-variant.rst Outdated Show resolved Hide resolved
hoomd/variant/box.py Outdated Show resolved Hide resolved
Moves the scalar variant classes to a separate module, but imports them
in hoomd/variant/__init__.py so that they still live in the
hoomd.variant namespace. I did this as a workaround for python's import
behavior since I kept getting circular import errors when keeping the
scalar variant classes in the package's __init__.py file.
@joaander joaander added the validate Execute long running validation tests on pull requests label Jan 17, 2024
Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have some more suggestions to improve this.

Also, I plan to merge this after the 4.5.0 release with the expectation that we can add code using box variants (BoxResize, QuickCompress) in the same release that we introduce the variants.

hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/VectorVariant.h Show resolved Hide resolved
hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/variant/box.py Outdated Show resolved Hide resolved
hoomd/VectorVariant.h Outdated Show resolved Hide resolved
hoomd/variant/box.py Outdated Show resolved Hide resolved
hoomd/variant/box.py Outdated Show resolved Hide resolved
hoomd/variant/box.py Outdated Show resolved Hide resolved
hoomd/variant/__init__.py Outdated Show resolved Hide resolved
hoomd/variant/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should either rename variant_like to scalar_varient_like or make very clear it is for scalar variants.

The code looks good generally. Just left a few comments and probably won't be able to get back to this.

hoomd/VectorVariant.h Show resolved Hide resolved
/// Method to enable unit testing of C++ variant calls from pytest
std::array<Scalar, 6> testVectorVariantBoxCall(std::shared_ptr<VectorVariantBox> t, uint64_t step)
{
return (*t)(step);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer t->operator()(step), but feel free to leave alone.

hoomd/variant/box.py Outdated Show resolved Hide resolved
hoomd/variant/box.py Show resolved Hide resolved
hoomd/pytest/test_box_variant.py Outdated Show resolved Hide resolved
hoomd/pytest/test_box_variant.py Outdated Show resolved Hide resolved
hoomd/pytest/test_box_variant.py Outdated Show resolved Hide resolved
Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I implemented a few minor suggestions directly on the branch. This is good to merge now.

@joaander joaander merged commit d4349bf into trunk-minor Feb 28, 2024
40 checks passed
@joaander joaander deleted the feature/vector-variant branch February 28, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validate Execute long running validation tests on pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants