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

Add import for public API of optuna.terminator to optuna/terminator/__init__.py #4669

Merged
merged 5 commits into from
May 18, 2023

Conversation

cross32768
Copy link
Contributor

@cross32768 cross32768 commented May 11, 2023

Motivation

To make optuna.terminator related import more user-friendly, writing some import sentence to optuna/terminator/__init__.py is needed.

For example, current RegretBoundEvaluator requires

from optuna.terminator.improvement.evaluator import RegretBoundEvaluator

This PR make it more user-friendly as below:

from optuna.terminator import RegretBoundEvaluator

Description of the changes

Write some import sentence to optuna/terminator/__init__.py and changes import-style in tests and visualization to make it simpler.

@cross32768
Copy link
Contributor Author

cross32768 commented May 12, 2023

Discussion-needed point:

Below is the list of API that this PR make its import more user-friendly (they can be imported by from optuna.terminator ). Is this reasonable choice?

  • TerminatorCallback
  • BaseErrorEvaluator
  • CrossValidationErrorEvaluator
  • report_cross_validation_scores
  • StaticErrorEvaluator
  • BaseImprovementEvaluator
  • RegretBoundEvaluator
  • BaseTerminator
  • Terminator

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2023

Codecov Report

Merging #4669 (d536f0c) into master (d199673) will increase coverage by 0.00%.
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    #4669   +/-   ##
=======================================
  Coverage   90.62%   90.63%           
=======================================
  Files         186      187    +1     
  Lines       14292    14302   +10     
=======================================
+ Hits        12952    12962   +10     
  Misses       1340     1340           
Impacted Files Coverage Δ
optuna/terminator/__init__.py 100.00% <100.00%> (ø)
optuna/visualization/_terminator_improvement.py 36.70% <100.00%> (ø)

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

@cross32768 cross32768 marked this pull request as ready for review May 15, 2023 03:00
@g-votte g-votte removed their assignment May 15, 2023
@HideakiImamura
Copy link
Member

@toshihikoyanase Could you review this PR?

Copy link
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 removed their assignment May 18, 2023
Copy link
Member

@toshihikoyanase toshihikoyanase left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. The change looks great. Let me note some offline discussions.

Alternative approach: Export sub-modules instead of individual classes

The PR contains many Evaluators in optuna.terminator, which suggests that perhaps we should consider exporting sub-modules.

from optuna.terminator import callback
from optuna.terminator import erroreval
from optuna.terminator import improvement
from optuna.terminator.terminator import BaseTerminator
from optuna.terminator.terminator import Terminator

We decided against this approach, primarily because most users typically use just a few:

from optuna.terminator import report_cross_validation_scores, TerminatorCallback

If we were to only export sub-modules, the import statements would likely become more complex, as shown here:

from optuna.terminator.errorval import report_cross_validation_scores
from optuna.terminator.callback import TerminatorCallback

Avoid exporting base classes

It's important to note that base classes, such as BaseTerminator and BaseImprovementEvaluator, are still experimental. As a result, we might introduce some significant changes in future versions. If that happens, any terminators defined by users won't work with the updated versions. To prevent this, we may choose not to export these base classes to discourage users from defining their own classes.

Conversely, we recognize the potential for considerable improvements to our APIs by creating user-defined terminators. So we're eager to promote the use of these base classes and highly appreciate any user feedback.

@toshihikoyanase
Copy link
Member

We can see optuna.terminator.callback in the docstring of Terminator, but we can update it after #4675.

@toshihikoyanase toshihikoyanase added this to the v3.2.0 milestone May 18, 2023
@toshihikoyanase toshihikoyanase added the feature Change that does not break compatibility, but affects the public interfaces. label May 18, 2023
@toshihikoyanase toshihikoyanase merged commit cde5850 into optuna:master May 18, 2023
32 checks passed
Alnusjaponica added a commit to Alnusjaponica/optuna that referenced this pull request May 18, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants