Skip to content

Dissect trial.py.#1210

Merged
HideakiImamura merged 4 commits intooptuna:masterfrom
himkt:dissect-trial
May 13, 2020
Merged

Dissect trial.py.#1210
HideakiImamura merged 4 commits intooptuna:masterfrom
himkt:dissect-trial

Conversation

@himkt
Copy link
Copy Markdown
Member

@himkt himkt commented May 6, 2020 •
edited
Loading

Close #1072.

Motivation

This PR separates trial.py into trials/*.py (since its codebase is becoming huge) such that it keeps the API unchanged.

Description of the changes

  • created optuna/trials
  • changed optuna/trial.py: now it only imports modules in optuna/trials
  • introduced util.py: contain _adjust_discrete_uniform_high. I think it needs a discussion about naming and placing

Concerns

  • util.py: naming for the module and placing
  • script structure

Another topic

In the source issue, we also discussed about changing API.
It's reasonable for me to make another PR about rethinking API since this PR becomes relatively huge.

Memo

crcrpar
crcrpar reviewed May 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@crcrpar crcrpar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is there any discussion about introducing trials namespace?

@crcrpar crcrpar added the code-fix Change that does not change the behavior, such as code refactoring. label May 7, 2020
@himkt
Copy link
Copy Markdown
Member Author

himkt commented May 8, 2020

I think there is no discussion yet, I'd appreciate it if developers give me some comments.
@hvy Could you please give me some advice?

@hvy
Copy link
Copy Markdown
Member

hvy commented May 8, 2020

I sort of started one here #1072 (comment). To add, introducing trials is a quite big change and if we are to do so, we should probably at least keep aliases for backward compatibility. However, in practice, I image that many end-users actually won't be affected as they don't have to import optuna.trial. For users extending Optuna, there's an entire different story and hence the backward compatibility.

@HideakiImamura
Copy link
Copy Markdown
Member

HideakiImamura commented May 8, 2020

Thank you for your proposal! I totally agree with your idea to diet optuna.trial.py. But, I worry about extending namespaces optuna.trials. In my personal opinion, we should not extend so and let developers just use all trial classes as optuna.trial.Trial. I believe it is developer-friendly.

@himkt
Copy link
Copy Markdown
Member Author

himkt commented May 9, 2020

To add, introducing trials is a quite big change and if we are to do so, we should probably at least keep aliases for backward compatibility

I thought the concern is only about backward compatibility and added a trick to keep them here, which means optuna.trial is available, I think.

However, the availability of both optuna.trial and optuna.trials is confusing and
I agree with @HideakiImamura that we shouldn't extend namespace now.

I'll make the change to keep backward compatibility without extending namespace.
I want to discuss about rethinking API in another issue or PR.
Thanks, @hvy @HideakiImamura!

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 9, 2020 •
edited
Loading

Codecov Report

Merging #1210 into master will decrease coverage by 0.29%.
The diff coverage is 78.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
- Coverage   85.87%   85.58%   -0.30%     
==========================================
  Files          86       92       +6     
  Lines        6561     6616      +55     
==========================================
+ Hits         5634     5662      +28     
- Misses        927      954      +27
Impacted Files Coverage Δ
optuna/trial/base.py 56.89% <56.89%> (ø)
optuna/trial/fixed.py 79.74% <79.74%> (ø)
optuna/trial/frozen.py 85.00% <85.00%> (ø)
optuna/trial/__init__.py 100.00% <100.00%> (ø)
optuna/trial/state.py 100.00% <100.00%> (ø)
optuna/trial/trial.py 90.27% <100.00%> (ø)
optuna/trial/util.py 100.00% <100.00%> (ø)
... and 3 more

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 8b5e54a...a3ad458. Read the comment docs.

@crcrpar crcrpar added this to the v1.5.0 milestone May 11, 2020
@hvy hvy added the optuna.trial Related to the `optuna.trial` submodule. This is automatically labeled by github-actions. label May 13, 2020
@HideakiImamura HideakiImamura mentioned this pull request May 13, 2020
Merged
HideakiImamura
HideakiImamura approved these changes May 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura left a comment

Choose a reason for hiding this comment

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

LGTM!

@HideakiImamura HideakiImamura merged commit 799e220 into optuna:master May 13, 2020
@hvy hvy mentioned this pull request May 14, 2020
Merged
@himkt himkt deleted the dissect-trial branch June 20, 2020 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HideakiImamura HideakiImamura HideakiImamura approved these changes

+1 more reviewer

@crcrpar crcrpar crcrpar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-fix Change that does not change the behavior, such as code refactoring. optuna.trial Related to the `optuna.trial` submodule. This is automatically labeled by github-actions.

Projects

None yet

Milestone

v1.5.0

Development

Successfully merging this pull request may close these issues.

Separate trial.py into trials/xxx_trial.py.

5 participants

@himkt @hvy @HideakiImamura @codecov-io @crcrpar