Skip to content

Resample when a point is out-of-bounds#11

Merged
pengowen123 merged 1 commit intopengowen123:masterfrom
MenloSystems:feature/out-of-bounds-resample
May 19, 2025
Merged

Resample when a point is out-of-bounds#11
pengowen123 merged 1 commit intopengowen123:masterfrom
MenloSystems:feature/out-of-bounds-resample

Conversation

@msarehn
Copy link
Contributor

@msarehn msarehn commented Apr 22, 2025

Adds resampling to keep points within defined bounds. Compared to other constraint-handling methods, this can speed up convergence significantly. It's not a one-size-fits-all solution, however.

Also supports a resampling limit in order to not get stuck resampling on the boundary's edges.

self
}

/// Changes the bounds from the defalt value. Vector length must match the number of dimensions.
Copy link
Owner

Choose a reason for hiding this comment

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

This condition should be checked in CMAES::new, and can you add a test for it as well? Also, minor typo: defalt -> default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, when I go the dyn Constraints way, this will not be possible anymore. We don't know what the concrete Constraints require. I could probably try to downcast do Bounds specifically, but that would be a code smell I think.

What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

A clean way to verify constraints would be adding a method like fn are_constraints_valid(&CMAESOptions) -> bool to Constraints and calling it in CMAES::new. I'm fine with leaving this out for now because the code providing the constraints can ensure their validity.

@pengowen123
Copy link
Owner

Thanks for the PR! I left some minor comments, and can you also run rustfmt?

This feature is definitely useful, and the implementation looks good as well. I'm wondering though if it would be better to generalize Bounds to a trait like this:

trait Constraints {
    fn meets_constraints(x: &DVector<f64>) -> bool;
}

Sampler would store a Box<dyn Constraints>, and the existing Bounds can simply implement the trait. The trait enables support for more complex constraints at the cost of a virtual function call per point sampled. I imagine this cost would be small though, especially relative to expensive objective functions. What do you think?

@msarehn
Copy link
Contributor Author

msarehn commented May 12, 2025

Went the dyn Constraints way now. Also ran rustfmt. Ready for re-review.

@msarehn msarehn requested a review from pengowen123 May 12, 2025 12:02
@pengowen123
Copy link
Owner

Looks great! I left one more comment, but I'll merge this as soon as it's resolved.

@msarehn msarehn force-pushed the feature/out-of-bounds-resample branch from 4d6f94f to 66c52d1 Compare May 19, 2025 08:25
@msarehn
Copy link
Contributor Author

msarehn commented May 19, 2025

Squashed the fixups.

@msarehn msarehn requested a review from pengowen123 May 19, 2025 08:25
@pengowen123
Copy link
Owner

Thanks!

@pengowen123 pengowen123 merged commit 2bb2b3f into pengowen123:master May 19, 2025
1 check failed
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