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 docstrings to optuna.termintor #4675

Merged
merged 19 commits into from
May 25, 2023

Conversation

Alnusjaponica
Copy link
Collaborator

Motivation

Resolve #4598

Description of the changes

  • Add docstrings to
    • TerminatorCallback (optuna/terminator/callback.py)
    • CrossValidationErrorEvaluator (optuna/terminator/erroreval.py)
    • report_cross_validation_scores (optuna/terminator/erroreval.py)
    • StaticErrorEvaluator (optuna/terminator/erroreval.py)
  • Fix docstring of Terminator (optuna/terminator/terminator.py)
  • Add .rst files for terminator documentation

@Alnusjaponica
Copy link
Collaborator Author

Alnusjaponica commented May 14, 2023

There are two points to discuss regarding this PR:

  • To avoid errors in doctest, the output destination of the print statement in the Terminator docstring is set to standard error. However, I don't think this is a good approach. What should be done instead?
    -> I replaced print with logging.info for now.
  • Is the current module hierarchy and the correspondence with the .rst files correct?
    -> I am going to update this PR or submit another follow-up PR according to how Add import for public API of optuna.terminator to optuna/terminator/__init__.py #4669 progresses.

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2023

Codecov Report

Merging #4675 (a0a22b4) into master (cde5850) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ 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    #4675      +/-   ##
==========================================
- Coverage   90.62%   90.60%   -0.03%     
==========================================
  Files         187      187              
  Lines       14322    14286      -36     
==========================================
- Hits        12980    12944      -36     
  Misses       1342     1342              
Impacted Files Coverage Δ
optuna/terminator/callback.py 100.00% <ø> (ø)
optuna/terminator/erroreval.py 97.67% <ø> (ø)
optuna/terminator/improvement/evaluator.py 89.83% <ø> (ø)
optuna/terminator/terminator.py 96.66% <ø> (ø)

... and 6 files with indirect coverage changes

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

@HideakiImamura
Copy link
Member

@eukaryo @contramundum53 Could you review this PR?

@HideakiImamura
Copy link
Member

Let me reassign the reviewer from @eukaryo to me, since he has day-off.

@HideakiImamura
Copy link
Member

I think we need some docstrings for classes/functions listed in https://github.com/optuna/optuna/blob/master/optuna/terminator/__init__.py. Concretely, it would be great to add docstrings of BaseErrorEvaluator, BaseImprovementEvaluator, RegretBoundEvaluator, and BaseTerminator. Could you work on them?

terminator:
A terminator object which determines whether to terminate the optimization by
assessing the room for optimization and statistical error. Defaults to a
:class:`~optuna.terminator.Terminator` object with default
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should explain here what the default improvement_evluator and error_evaluator does, because for most users this is the only interface to touch?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should not explain them here. It is enough to explain them in the document of Terminator.

:class:`~optuna.terminator.Terminator` object with default
improvement_evaluator and error_evaluator.

Example:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add another example to show how optuna.terminator module can be used when no cross-validation is performed?

Copy link
Member

Choose a reason for hiding this comment

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

I think it can be done as a follow-up.

Alnusjaponica and others added 2 commits May 22, 2023 12:48
Co-authored-by: contramundum53 <contramundum53@outlook.com>
Co-authored-by: contramundum53 <contramundum53@outlook.com>
@Alnusjaponica
Copy link
Collaborator Author

@HideakiImamura Sure! I'll add them.

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 except for @HideakiImamura 's comment.

@Alnusjaponica
Copy link
Collaborator Author

@HideakiImamura @contramundum53
I added docstrings of base classes and RegretBoudEstimator. PTAL.

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 merged commit 5cef676 into optuna:master May 25, 2023
31 checks passed
@HideakiImamura HideakiImamura added the document Documentation related. label May 25, 2023
@HideakiImamura HideakiImamura added this to the v3.2.0 milestone May 25, 2023
@Alnusjaponica Alnusjaponica deleted the terminator-docs branch May 25, 2023 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add docstrings for Optuna Terminator APIs
5 participants