Skip to content

Refactor integrator #79

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Refactor integrator #79

wants to merge 16 commits into from

Conversation

mayukh33
Copy link
Contributor

@mayukh33 mayukh33 commented Jun 4, 2024

No description provided.

@mayukh33 mayukh33 requested a review from mphoward June 5, 2024 14:51
Copy link
Collaborator

@mphoward mphoward left a comment

Choose a reason for hiding this comment

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

Thanks, this is going well too! Can you please take care of the small comments here, and then I can help you with the CMake setup for compiling these so you can also work on the GPU code.

@@ -90,10 +84,6 @@ template<class FlowField> class PYBIND11_EXPORT TwoStepBrownianFlow : public Two
/*!
* \param noiseless If true, do not apply a random diffusive force
*/
void setNoiseless(bool noiseless)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This setter should not have been removed, please restore.

Comment on lines 176 to 177
.def("getFlowField", &BrownianFlow::getFlowField)
.def("getNoiseless", &BrownianFlow::getNoiseless);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is how you make a property. flow_field should be read-only, but noiseless can be mutable.

Suggested change
.def("getFlowField", &BrownianFlow::getFlowField)
.def("getNoiseless", &BrownianFlow::getNoiseless);
.def_property_readonly("flow_field", &BrownianFlow::getFlowField)
.def_property("noiseless", &BrownianFlow::getNoiseless, &BrownianFlow::setNoiseless);

}
} // end namespace detail
} // end namespace azplugins

} // end namespace hoomd
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
} // end namespace hoomd
} // end namespace hoomd

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please apply the same changes to this file as for the BD one.

src/module.cc Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will help you write some machinery to compile these.

@mayukh33 mayukh33 force-pushed the refactor/integrator branch from eb25c1e to 4bdc2e6 Compare June 5, 2024 20:54
Base automatically changed from refactor/flow to azplugins-v1 June 5, 2024 21:47
@mayukh33 mayukh33 requested a review from mphoward June 6, 2024 17:25
Base automatically changed from azplugins-v1 to main July 26, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants