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.

Sign up for GitHub

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

Jump to bottom

Remove ordered_dict argument from IntersectionSearchSpace #4846

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Remove ordered_dict argument from IntersectionSearchSpace #4846

merged 1 commit into from
Aug 1, 2023

Conversation

not522
Copy link
Member

@not522 not522 commented Jul 28, 2023

Motivation

By #4838, OrderedDict is removed from Optuna's code. Therefore the ordered_dict argument is now inappropriate.

Description of the changes

This PR removes the ordered_dict argument and makes the returned dict always sorted.

@github-actions github-actions bot added optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions. optuna.importance Related to the `optuna.importance` submodule. This is automatically labeled by github-actions. labels Jul 28, 2023
@not522
Copy link
Member Author

not522 commented Jul 31, 2023

@Alnusjaponica @contramundum53 Could you review this PR?

@not522 not522 added the compatibility Change that breaks compatibility. label Jul 31, 2023
@Alnusjaponica
Copy link
Collaborator

Alnusjaponica commented Jul 31, 2023 •
edited

LGTM.
This is just a question but I just want to double check

  • whether you intentionally left samplers/_search_space/intersection.py unchanged
  • and why it is necessary to sort items in the first place.

contramundum53
contramundum53 approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@contramundum53 contramundum53 left a comment

Choose a reason for hiding this comment

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

LGTM.

@contramundum53 contramundum53 added this to the v3.3.0 milestone Aug 1, 2023
@contramundum53 contramundum53 merged commit fa19042 into optuna:master Aug 1, 2023
33 checks passed
@not522 not522 deleted the remove-ordered-dict-arg branch August 1, 2023 02:33
@not522
Copy link
Member Author

not522 commented Aug 1, 2023

whether you intentionally left samplers/_search_space/intersection.py unchanged

Yes, it's already deprecated, so no need to update.

why it is necessary to sort items in the first place.

importance features assumes that the order of parameters is fixed. The current implementation sorts trial.distributions but it is not the specification. (ref #4775)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@contramundum53 contramundum53 contramundum53 approved these changes

Assignees

@contramundum53 contramundum53

@Alnusjaponica Alnusjaponica

Labels
compatibility Change that breaks compatibility. optuna.importance Related to the `optuna.importance` submodule. This is automatically labeled by github-actions. optuna.integration Related to the `optuna.integration` submodule. This is automatically labeled by github-actions.
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@not522 @Alnusjaponica @contramundum53