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

Remove abstractmethod decorator from get_trial_id_from_study_id_trial_number #3909

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

c-bata
Copy link
Member

@c-bata c-bata commented Aug 22, 2022

Motivation

Revert changes in #3870.

Description of the changes

@knshnb told me that get_trial_id_from_study_id_trial_number is actually not an abstractmethod though it raises NotImplementedError. When NotImplementedError is raised, following lines handles an exception and returns trial_id.

try:
trial_id = study._storage.get_trial_id_from_study_id_trial_number(
study._study_id, trial_number
)
except NotImplementedError as e:
warnings.warn(
"Study.tell may be slow because the trial was represented by its number but "
f"the storage {study._storage.__class__.__name__} does not implement the "
"method required to map numbers back. Please provide the trial object "
"to avoid performance degradation."
)
trials = study.get_trials(deepcopy=False)
if len(trials) <= trial_number:
raise ValueError(
f"Cannot tell for trial with number {trial_number} since it has not been "
"created."
) from e
trial_id = trials[trial_number]._trial_id

I think we should implement the default logic in BaseSampler. However, this PR revert #3870 for now, since we plan to release v3.0 next week.

@github-actions github-actions bot added the optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. label Aug 22, 2022
@knshnb knshnb self-assigned this Aug 22, 2022
Copy link
Member

@knshnb knshnb left a comment

Choose a reason for hiding this comment

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

LGTM!

@toshihikoyanase toshihikoyanase added the feature Change that does not break compatibility, but affects the public interfaces. label Aug 22, 2022
@toshihikoyanase toshihikoyanase added this to the v3.0.0 milestone Aug 22, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #3909 (457e9f8) into master (5a1bb75) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3909      +/-   ##
==========================================
- Coverage   90.58%   90.57%   -0.02%     
==========================================
  Files         164      164              
  Lines       12601    12600       -1     
==========================================
- Hits        11415    11412       -3     
- Misses       1186     1188       +2     
Impacted Files Coverage Δ
optuna/storages/_base.py 71.55% <ø> (-0.26%) ⬇️
optuna/integration/botorch.py 97.37% <0.00%> (-0.88%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@knshnb knshnb merged commit 7bebfd6 into optuna:master Aug 22, 2022
@c-bata c-bata deleted the revert-3870 branch August 22, 2022 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Change that does not break compatibility, but affects the public interfaces. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants