Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bugfix cec2022 benchmarks #16
Bugfix cec2022 benchmarks #16
Changes from 1 commit
0acff8c
42d0554
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@firestrand,
Could you please add a 'shifted' parameter to this type of function? Kindly avoid fixed the shifted value within the function. This will make the function more reusable.
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.
@firestrand ,
This function too, you can add parameter for it.
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.
Please confirm you want parameterized after reading.
I specifically didn't go with a parameterized because I was thinking it would require either an if condition to check the parameter and shift or not, or always perform the shift just with a 0 for some cases if no shift is desired. While small this could introduce some extra processing. So having a shifted override function which calls the base unshifted seemed like the best performance vs clean approach.
That said if you feel going with a parameterized approach matches the codebase better I can go that direction, and if so prefer passing the shift value as 1, -1 etc. with a default of 0 instead of an
if
check.Thoughts?
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.
@firestrand,
Yes, please set up the parameter name that match with function name. Also set up the default value for it. You don't need to check if else condition here. Your function will be reused more in future.
Other sections looking good. I will accept this PR after you do it. Thanks
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.
@thieu1995
Started to make the change and it doesn't seem right for the codebase.
Changing the signature to something like
def happy_cat_func(x, shift=0.0)
works fine for when the benchmark is either calling the shifted or unshifted directly. ie.happy_cat_func(x, shift=-1.0)
orhappy_cat_func(x)
. However with the composite benchmarks the code does something likeself.g2 = operator.happy_cat_func
then in the evaluate we will getself.g2(mz[self.n1:self.n2], shift=-1.0)
which will be different from the other self.g1,g3 calls.I considered a functional approach, which would be something like
def happy_cat_func(shift=0.0): def inner(x): <existing logic>
which would then transform theself.g2 = operator.happy_cat_func(-1.0)
and the evaluate code would be the same. But then the functions that currently call the function directly would look likeoperator.happy_cat_func()(x)
which would be different.The last alternative is leaving things the way I have with a separate function definition for the shifted case so the signatures in the benchmark stay the same.
So advise option 1, 2, or 3 given this information.
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.
@firestrand ,
I forgot to tell you when I defined self.g1 =..., self.g2=... in the init function. It is just for short code.
Now because you have parameter in the function, then you don't have to define g1, g2,... function.
Just write full function in evaluate().
For example, return operator.happy_cat_func((mz[self.n1:self.n2], shift=1.0) + operator.katsuura_func(mz[self.n1:self.n2], shift=-1.0,...) like that.