Fix unexpected recommendation #951
Conversation
facebook-github-bot
added
the
CLA Signed
|
@teytaud not sure how to solve this, maybe use the average as default to return the value ? this seems to be the expected behavior. Returning the pessimistic best as a default even for non-noisy optimization is confusing |
In the deterministic setting yes we should return the average. |
jrapin
changed the title
| name = "minimum" if self.parametrization.descriptors.deterministic_function else "pessimistic" | ||
| return self.current_bests[name].parameter |
There was a problem hiding this comment.
@teytaud this is a drastic change in my opinion, you may want to crosscheck it in the xps
There was a problem hiding this comment.
I think it is safe but yes I'll check xps... complicated though are the absolute indicators are just new.
There was a problem hiding this comment.
I guess we can wait a bit to merge if it is safer
There was a problem hiding this comment.
or I provide a fix for the noisy variants
| @@ -395,7 +395,7 @@ def _update_archive_and_bests(self, candidate: p.Parameter, loss: tp.FloatLoss) | |||
| # update current best records | |||
| # this may have to be improved if we want to keep more kinds of best losss | |||
|
|
|||
| for name in ["optimistic", "pessimistic", "average"]: | |||
| for name in self.current_bests: | |||
There was a problem hiding this comment.
ah yes cool as a shortcut :-)
| return score | ||
|
|
||
|
|
||
| def test_recommendation_correct() -> None: |
There was a problem hiding this comment.
A possible test would be the example in the issue.
That said, I find the fix quite cool and convincing.
There was a problem hiding this comment.
Actually that is the same test, just simplified a bit, but it does exactly the same
Types of changes
Motivation and Context / Related issue
#947
What happens:
How Has This Been Tested (if it applies)
Checklist