-
Notifications
You must be signed in to change notification settings - Fork 44
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
feat (algo): Add MAP-Elites Low-Spread #152
feat (algo): Add MAP-Elites Low-Spread #152
Conversation
Hi @btjanaka ! Thank you very much for this (super well-documented) PR. I added @Egiob and @manon-but-yes as reviewers for this PR; they are probably the best people to judge it. I had an overview of the code and it looks great! I have mainly one high-level comment regarding the implementation of the reevaluation scoring function (and where it should be implemented). I will add it as a comment. |
Hi @Lookatator, thank you for taking a look at this PR! I addressed your comment on the pairwise distance calculation, but I do not see the comment on scoring function evaluation. Did you make the comment in the tests or the notebook? (the scoring function is in both those places). |
Hi guys! I will do a pass by the end of the week 😄 |
Sorry for the late reply @btjanaka , I did not have the time to post it yesterday, and I also was hesitating to add additional comments regarding the repertoire management (I think the calculation of the mode and of the spread should be done in the algorithm, and the repertoire addition condition should be receiving the same kinds of inputs as the |
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.
Hey @btjanaka, thanks for the very-clean PR, amazing work! I've left a couple of small suggestions, but all good for me 👍
from qdax.types import Centroid, Descriptor, ExtraScores, Fitness, Genotype, Spread | ||
|
||
|
||
def _dispersion(descriptors: jnp.ndarray) -> jnp.ndarray: |
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.
Minor suggestion: perhaps these functions belong elsewhere in a utils/ file ? I feel like they can be used by other methods as scores or metrics
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 if it becomes something used in multiple places, perhaps it can then be refactored? Since it is a private function, it seems it can be moved around pretty easily.
Thank you @Egiob! I've resolved all your comments except for the one about moving |
@Lookatator Is there anything else I should do to get this PR merged? |
Hi @btjanaka thank you for this PR! Apologies for the delay I just got back to work. |
79939ee
into
adaptive-intelligent-robotics:feat/mels
@btjanaka I merged it into a local branch to check potential style issues before merging into develop :) |
feat(algo): Add MAP-Elites Low-Spread (#152)
Related issues: [refer to issues] Resolves #151
[Introduce the overall change made in the PR and list the associated modifications below]
Add MAP-Elites Low-Spread: https://dl.acm.org/doi/abs/10.1145/3583131.3590433
This PR introduces:
MELSRepertoire
inmels_repertoire.py
mels_repertoire_test.py
MELS
inmels.py
mels_test.py
Spread
type intypes.py
multi_sample_scoring_function
insampling.py
Questions
n_evals
fitnesses and descriptors in the repertoire, or should we just store the aggregated fitness and descriptors? We can also store both the aggregate and the separate values. Storing separate values would require passing inn_evals
toMELSRepertoire.init_default
so that we can define the shapes offitnesses
anddescriptors
.Checks
Future improvements
[List here potential observations made and/or improvements that could be made in the future. If relevant, open issues for those.]