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

Sort params on fetch #4775

Merged
merged 7 commits into from
Jul 7, 2023
Merged

Conversation

contramundum53
Copy link
Member

Motivation

On RDBStorage, param order is not conserved in user API, although the database stores the order information in the form of param_id. This creates issues (e.g. #4771) for certain samplers that use the order of parameters to analyze the parameter dependency relation in dynamic search space. We ensure that RDBStorage returns the parameters in the order they are suggested.

Description of the changes

  • Ensure that trial.params is in the order they are suggested, in RDBStorage.
  • Test that for all storages, trial.params are in the right order.

@github-actions github-actions bot added the optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. label Jun 29, 2023
@not522
Copy link
Member

not522 commented Jun 30, 2023

@gen740 Could you review this PR?

@contramundum53
Copy link
Member Author

contramundum53 commented Jul 4, 2023

After a discussion, we decided not to require all storage classes to preserve the order of parameters and now #4771 can be solved independently by #4782. However, preserving the order of parameters is still helpful when it's possible, so I'll leave this PR open.

@codecov-commenter
Copy link

Codecov Report

Merging #4775 (22d5f92) into master (1b0c6a8) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #4775      +/-   ##
==========================================
- Coverage   90.75%   90.73%   -0.02%     
==========================================
  Files         190      190              
  Lines       14468    14470       +2     
==========================================
  Hits        13130    13130              
- Misses       1338     1340       +2     
Impacted Files Coverage Δ
optuna/storages/_rdb/storage.py 94.33% <100.00%> (+0.02%) ⬆️

... and 2 files with indirect coverage changes

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

Copy link
Member

@not522 not522 left a comment

Choose a reason for hiding this comment

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

LGTM

@not522 not522 removed their assignment Jul 5, 2023
@not522 not522 added the feature Change that does not break compatibility, but affects the public interfaces. label Jul 5, 2023
Copy link
Collaborator

@gen740 gen740 left a comment

Choose a reason for hiding this comment

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

LGTM!

@not522 not522 merged commit 67b84d1 into optuna:master Jul 7, 2023
32 checks passed
@not522 not522 added this to the v3.3.0 milestone Jul 7, 2023
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