-
Notifications
You must be signed in to change notification settings - Fork 79
Create criteria and termination data structure #1293
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1293 +/- ##
===========================================
+ Coverage 81.15% 81.45% +0.29%
===========================================
Files 58 59 +1
Lines 5911 6033 +122
===========================================
+ Hits 4797 4914 +117
- Misses 1114 1119 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CalCraven
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few minor suggestions.
mbuild/path/criteria.py
Outdated
| # Check required criteria first | ||
| if all([c.is_met() for c in self.required]): | ||
| return True | ||
| if any([c.is_met() for c in self.not_required]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a quick change in name:
I think instead of "required" and "not_required", these criteria are better classified as "immediate_termination", and "partial_termination".
But, maybe in order to utilize the True False tag, you could use "is_immediate_terminator", or even "is_instant_terminator". And if False, check that All are met, if True, check if Any are met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that choosing clear variable names here is important. The actual user-facing classes use the required_to_end variable. The self.required and self.not_required aren't ever exposed variables/parameters. For now, I'll rename these to match the parameters in the individual classes to avoid any confusion.
IMO, I'm not sure if partial_termination and immediate_termination is really more clear than required_to_end. I guess it depends on from which angle you look at it from.
WallTime does not need to be met for a random walk to end, so required_to_end is False or WallTime should trigger an instant termination, so is_instant_terminator. is True.
I think either approach to the variable naming has pros and cons. I imagine we can build in some more robust and improved functionality here (see one of my comments below) that will address this. I think we should leave it as required_to_end for now.
mbuild/path/criteria.py
Outdated
| return current_distance <= self.distance + self.tolerance | ||
|
|
||
|
|
||
| class FinalCoordinate(Criterion): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| class FinalCoordinate(Criterion): | |
| class DestinationCoordinate(Criterion): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might just remove this one. It's essentially the same as WithinCoordinate with distance = 0
mbuild/path/criteria.py
Outdated
| return np.linalg.norm(self.final_coordinate - last_site) <= self.tolerance | ||
|
|
||
|
|
||
| class PairDistance(Criterion): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this like checking all "_UL" particles for distance to the current RandomWalk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that this will trigger a termination if the last step is within a certain distance to a certain particle type. So, you could set up a series of Terminators that do something like "Make a random walk of at least 10 steps, then stop once you are within 0.3nm of a type B site"
| import numpy as np | ||
|
|
||
|
|
||
| class Termination: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future TODO: may be worthwhile to also have a Trigger, where each Criterion triggers in different numbers of steps. I don't think it's necessary for now though, just something to think about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this brings up a good point. Some of these will always be True once triggered (NumSites) while the others will be dynamic (WithinCoordinate, EndtoEndDistance, etc..). Right now, there is no logic or dependence on the order of using multiple terminators. There could be some performance improvement and additional flexibility if there were some way to set precedence. Like, don't start checking for RadiusOfGyration until NumSites has triggered to True.
I think this could be done in a future PR, but there are still lots of interesting use cases for designing this kind of data structure that we could address.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
So, with introducing the termination functionality, now in some cases we won't know the actual walk length ahead of time. To keep coordinates as arrays instead of lists, I added some functionality that builds initial coordinate arrays in pre-defined chunk sizes. If the walk doesn't use the whole chunk, it's trimmed once it's complete. If the walk needs more room then the coordinates are extended by another chunk. |
|
Do we want some termination criteria to remain defined as their own parameters in Or we could just go all-in on |
|
I wonder if it would be useful to have some dictionary based class method that automatically creates the Terminator classes? or I guess we could have a class method with parameters for all of the supported which then creates all of the individual This wouldn't work with Terminator classes that have more than 1 parameter though, like |
PR Summary:
This PR adds a modular termination system for random walks, designed to complement the Bias class (#1292) and provide flexibility in designing specific paths, such as semi-crystalline lamellae or cross-links. Termination criteria are not hard-coded into the random walk class, allowing users to define new criteria as needed or handle edge cases specific to their system.
The main ideas and features are:
Example use case: A walk could be required to come within 0.1 nm of a target coordinate, while also enforcing a minimum walk length. Meanwhile, safety criteria like maximum attempts or wall time can be included as well to ensure the walk does not run indefinitely.
Next steps: Replace the existing while loop with calls to the Termination instance provided when the random walk is initiated. Once this is done, we can see if there is a significant performance hit. I don't think there will be, especially since most of these criteria are not doing large iterations through the complete set of coordinates.
@CalCraven I'm open to any design and/or implementation feedback or ideas you might have.
PR Checklist