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

Do not get stuck due to adaptive capping #749

Merged
merged 5 commits into from
Sep 23, 2021

Conversation

filipbartek
Copy link
Contributor

If the incumbent has all the instance-seed pairs covered
and the challenger is ruled out due to adaptive capping,
we cancel all the runs on the current challenger
to proceed to the next challenger.

This is a possibly dirty quick fix.
I am not well acquainted with the code.
I welcome suggestions for improvements of the fix.

Resolves #748

If the incumbent has all the instance-seed pairs covered
and the challenger is ruled out due to adaptive capping,
we cancel all the runs on the current challenger
to proceed to the next challenger.
@renesass
Copy link
Collaborator

renesass commented Aug 5, 2021

Thanks for your contribution! We'll look into this shortly.

@renesass renesass requested a review from mlindauer August 5, 2021 11:53
@mlindauer
Copy link
Contributor

Hi,

this looks good and the change makes sense in principle. However, I still don't fully see why the bug happens.
@renesass could you please run the example from @filipbartek 's issue and check that everything is fine now.

@franchuterivera do you have any comment on this? I think this bug was introduced because of the new IntensifierStage, which I'm not super familiar with.

@renesass
Copy link
Collaborator

renesass commented Aug 5, 2021

I ran @filipbartek code on both the old and the new code and got the same results. However, we should investigate why this problem happens in the first place. Since more time is required to find the bugs, we can't solve it by the upcoming release.

Discard a challenger that is ruled out due to adaptive capping
immediately.
Do not wait for the incumbent to be saturated.
Once a challenger is ruled out due to adaptive capping,
it would always fail the adaptive cap test in the future.
@mlindauer
Copy link
Contributor

@franchuterivera @mfeurer could you please comment on this?
I don't understand why this change is required if we do a stage change anyway.
After we got not incumbent runs, I would expect that we could consider the same challenger again, since the adaptive capping allows now for more time.

@mlindauer
Copy link
Contributor

@filipbartek please redirect your PR to development. Other than new releases, there is never a PR directed to master.

# We prevent this challenger from running again so that a new challenger is targeted in the next iteration.
# Otherwise, the current challenger would be ruled out by adaptive capping again.
self.continue_challenger = False
self.to_run = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply setting self.to_run = [] should suffice,
'self.continue_challenger' will be modified anyway under Intensification._process_racer_results (line 422)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having tested this on an example that I attached to issue #748, I agree that setting self.to_run suffices.
I have simplified the PR accordingly.

@filipbartek filipbartek changed the base branch from master to development August 23, 2021 08:55
@mfeurer
Copy link
Contributor

mfeurer commented Aug 24, 2021

So if I understand this correctly, the function _process_racer_results does not know that the evaluation of the current challenger should be stopped because adaptive capping mandates that (as that functionality was simply not programmed) and therefore the stage change doesn't happen properly. So what you're doing right now @filipbartek is "faking" the stop mechanism that _process_racer_results knows of. A cleaner solution would then be that we add a new argument to _process_racer_results which has the same effect as clearing self.to_run but is more descriptive. Is that correct @dengdifan?

Why the stage change wasn't sufficient is something I currently don't understand myself either :(

@dengdifan
Copy link
Contributor

adding an argument to _process_racer_results could also be an option.
Alternatively, we could add a docstring saying that "since the challenger fails to outperform the incumbent due to adaptive capping, we discard all the forthcoming runs"

Why the stage change wasn't sufficient is something I currently don't understand myself either :(

Intensifier will skip the 4th line of _process_racer_results if self.to_run is not empty. As self.to_run still contains elements, intensifier will continue evaluating the remaining instance-seed pairs in self.to_run.

@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #749 (215e3b6) into development (c4a745e) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #749      +/-   ##
===============================================
- Coverage        87.02%   87.01%   -0.02%     
===============================================
  Files               68       68              
  Lines             6221     6222       +1     
===============================================
  Hits              5414     5414              
- Misses             807      808       +1     
Impacted Files Coverage Δ
smac/intensification/intensification.py 95.43% <0.00%> (-0.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4a745e...215e3b6. Read the comment docs.

@dengdifan dengdifan merged commit 7cd6a98 into automl:development Sep 23, 2021
@filipbartek filipbartek deleted the issue/748 branch September 23, 2021 14:23
renesass added a commit that referenced this pull request Oct 20, 2021
* Documentation was updated thoroughly. A new theme with a new structure is provided and all pages have been updated. Also, the examples revised and up-to-date.
* Option to use an own stopping strategy using `IncorporateRunResultCallback`.
* Changed `scripts/smac` to `scripts/smac.py`.
* `README.md` updated.
* `CITATION.cff` added.
* Made `smac-validate.py` consistent with runhistory and tae. (#762)
* `minR`, `maxR` and `use_ta_time` can now be initialized by the scenario. (#775)
* `ConfigSpace.util.get_one_exchange_neighborhood`'s invalid configurations are ignored. (#773)
* Fixed an incorrect adaptive capping behaviour. (#749)
* Avoid the potential `ValueError` raised by `LocalSearch._do_search`. (#773)

Co-authored-by: dengdifan <33290713+dengdifan@users.noreply.github.com>
Co-authored-by: Filip Bártek <Filip.Bartek@hotmail.com>
Co-authored-by: Thomas Holvoet <tholvoet@telenet.be>
Co-authored-by: benjamc <75323339+benjamc@users.noreply.github.com>
Co-authored-by: Carolin Benjamins <benjamins@tnt.uni-hannover.de>
Co-authored-by: Marius Lindauer <marius.rks@googlemail.com>
Co-authored-by: eddiebergman <eddiebergmanhs@gmail.com>
Co-authored-by: Difan Deng <deng@dengster.tnt.uni-hannover.de>
Co-authored-by: dengdifan <difandeng@gmail.com>
Co-authored-by: Tim Ruhkopf <timruhkopf@gmail.com>
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.

Intensification may target the same configuration forever due to adaptive capping
5 participants