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

Implementation of Active CMAES #367

Merged
merged 37 commits into from
Aug 21, 2023
Merged

Conversation

SuvarshaChennareddy
Copy link
Contributor

This pull request is an initial implementation of Active CMAES, which is part of my Google Summer of Code (GSoC) project. Tests have not been added yet, but they will be included soon.

@SuvarshaChennareddy
Copy link
Contributor Author

SuvarshaChennareddy commented Jun 13, 2023

It's just the (Vanilla) CMAES tests that are failing. I'm fine tuning the parameters used in the tests.

@SuvarshaChennareddy SuvarshaChennareddy marked this pull request as ready for review June 13, 2023 21:50
#include <ensmallen_bits/function.hpp>

#ifndef NOT_EMPTY_TRANSFORMATION
#define NOT_EMPTY_TRANSFORMATION
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what this define is for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing this out! Now that I look at it, I don't think this define is required. The ACTIVE_CMAES_CMAES_IMPL_HPP define should be enough. I'll make changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking at this once more and I realize that if the define is removed and the user uses both Vanilla and Active CMAES, there will be double inclusion (which is why I added the include guard in the first place). The ACTIVE_CMAES_CMAES_IMPL_HPP define is indeed not enough.

Copy link
Member

Choose a reason for hiding this comment

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

It sounds like in that case that either NotEmptyTransfornation should be moved to its own file and included by both versions, or if the functionality differs here then perhaps it just needs a new name. I think I have understood the problem, let me know if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll move NotEmptyTransfornation to its own file.

@@ -20,31 +20,82 @@

#include <ensmallen_bits/function.hpp>

#ifndef NOT_EMPTY_TRANSFORMATION
#define NOT_EMPTY_TRANSFORMATION
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what this define is for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll be sure to update this too.

@SuvarshaChennareddy SuvarshaChennareddy marked this pull request as ready for review July 10, 2023 18:43
@zoq
Copy link
Member

zoq commented Aug 17, 2023

@mlpack-jenkins test this please

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

No more comments from my side, thanks for the contribution.

@SuvarshaChennareddy
Copy link
Contributor Author

Thank you, @zoq!
There is one thing I would like to address. Wouldn't removing the deprecation go against this. I also think we should make changes (fixing style and changing element types) the to Vanilla CMAES for consistency.

@zoq
Copy link
Member

zoq commented Aug 18, 2023

If we use BoundaryBoxConstraint as the default transformation policy wouldn't that be the same as using the other constructor?

@SuvarshaChennareddy
Copy link
Contributor Author

If we use BoundaryBoxConstraint as the default transformation policy wouldn't that be the same as using the other constructor?

Yes, but the default is EmptyTransformation.

@zoq
Copy link
Member

zoq commented Aug 18, 2023

Do you see any problem with using BoundaryBoxConstraint as the default for the constructor?

@zoq
Copy link
Member

zoq commented Aug 18, 2023

We could even provide another constructor for thee empty transformation.

@zoq
Copy link
Member

zoq commented Aug 18, 2023

Looking at it again, if we keep the constructor, it's backward compatible, since I can still use the "old" constructor.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@SuvarshaChennareddy
Copy link
Contributor Author

Looking at it again, if we keep the constructor, it's backward compatible, since I can still use the "old" constructor.

Yes, that's why it was marked as deprecated instead of being removed altogether. We could make BoundaryBoxConstraint the default constructor; however, CMA-ES itself isn't bounded. So, I think it would make more sense to keep EmptyTransformation as the default. Since it would be the default, keeping it deprecated would also make sense, wouldn't it? These are just my thoughts.

@zoq
Copy link
Member

zoq commented Aug 20, 2023

I think both interface are fine, the old interface is closer to what we provide for the other optimizers, so I like to keep it, the new one offers the ability to support more transformations so I like to keep it as well.

@SuvarshaChennareddy
Copy link
Contributor Author

Alright. We should probably modify Vanilla CMA-ES as well then (the element types should be changed as well for consistency).

@zoq
Copy link
Member

zoq commented Aug 21, 2023

Makes sense, I'll resolve the failing test and merge this PR.

@zoq zoq merged commit 1cfe984 into mlpack:master Aug 21, 2023
@zoq
Copy link
Member

zoq commented Aug 21, 2023

Thanks for the great contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants