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

Added sanitize optional parameter to minimize() #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MrJake222
Copy link

Used for example for resampling in RK-EDA.

@VicentePerezSoloviev
Copy link
Owner

Thank you for pointing this out. I really think that this could be a nice improvement to the algorithm itself. I fact, other libraries such as scipy integrate a similar option. In order to accept the pull request, may I ask you to add a couple of modifications?

  1. One of the main goals of the library is to implement the EDA approach the best modular and efficient possible. So, conditional sentences are a bit slow. Even more in Python. And even more if this is executed in each iteration of the algorithm. So, I see that you have added a callable argument to minimize function. I think that it could be interesting if by default callable is an empty function such as,
    def sanitize(): pass
    Then, you can always execute it, regardless it is passed by the used or not. Maybe this can be somehow checked at the beginning of the minimize function and then alway executed during the implementation.

  2. The second comment is to change the name of the argument. I see, for example in scipy.minimize that they call it callback function. May I ask you to change it so users are familiarized with the name itself?

Thank you again for the nice suggestion.

@VicentePerezSoloviev
Copy link
Owner

Hi @MrJake222 are you planning to keep working on this pull request? Thank you

@MrJake222
Copy link
Author

I've come up with my own (really simple) solution. No time to work on these now. Sorry. You can take over my code If you'd like.

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