Skip to content

Use TYPE_CHECKING for Study in samplers#5391

Merged
eukaryo merged 5 commits intooptuna:masterfrom
nabenabe0928:refactor/use-type-checking-for-study
Apr 17, 2024
Merged

Use TYPE_CHECKING for Study in samplers#5391
eukaryo merged 5 commits intooptuna:masterfrom
nabenabe0928:refactor/use-type-checking-for-study

Conversation

@nabenabe0928
Copy link
Copy Markdown
Contributor

Motivation

As circular references happen in samplers when removing multi_objective due to Study, I replaced Study in samplers with "Study" using TYPE_CHECKING.

Description of the changes

Replace all Study in samplers with "Study" using TYPE_CHECKING.

@nabenabe0928
Copy link
Copy Markdown
Contributor Author

nabenabe0928 commented Apr 12, 2024

Although I made everything "Study", we could also replace it with optuna.study.Study.

The former is more concise while the latter allows developers to use the direct link to Study in IDE.

I would like to ask @not522 which one is better.

@nabenabe0928 nabenabe0928 added the compatibility Change that breaks compatibility. label Apr 13, 2024
@nabenabe0928 nabenabe0928 added this to the v4.0.0 milestone Apr 13, 2024
@nabenabe0928
Copy link
Copy Markdown
Contributor Author

@eukaryo @not522 Could you review this PR?

@not522
Copy link
Copy Markdown
Member

not522 commented Apr 16, 2024

I recommend from __future__ import annotations instead of "Study" or optuna.study.Study.

@nabenabe0928 nabenabe0928 force-pushed the refactor/use-type-checking-for-study branch from 99852ec to fc106f5 Compare April 16, 2024 12:28


ObjectiveFuncType = Callable[[trial_module.Trial], Union[float, Sequence[float]]]
ObjectiveFuncType = Callable[["Trial"], Union[float, Sequence[float]]]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about moving it under if TYPE_CHECKING:?

Suggested change
ObjectiveFuncType = Callable[["Trial"], Union[float, Sequence[float]]]
ObjectiveFuncType = Callable[[Trial], Union[float, Sequence[float]]]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, I did not know that it works in this way!
I reflected your suggestion!

Co-authored-by: Naoto Mizuno <gobou522@gmail.com>
Copy link
Copy Markdown
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 Apr 17, 2024
@not522 not522 added code-fix Change that does not change the behavior, such as code refactoring. and removed compatibility Change that breaks compatibility. labels Apr 17, 2024
Copy link
Copy Markdown
Collaborator

@eukaryo eukaryo left a comment

Choose a reason for hiding this comment

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

LGTM.

@eukaryo eukaryo merged commit d272905 into optuna:master Apr 17, 2024
@eukaryo eukaryo removed their assignment Apr 17, 2024
@nabenabe0928 nabenabe0928 deleted the refactor/use-type-checking-for-study branch May 24, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-fix Change that does not change the behavior, such as code refactoring.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants