Add docstring to `BaseStorage` method interfaces. by ytsmiling · Pull Request #1175 · optuna/optuna

ytsmiling · 2020-04-28T01:45:31Z

Motivation

Storage class implementations for each backend handles error in a different manner. Additionally, undocumented return values force users to read implementations of each method. This PR addresses these issues as a step in #1170. Any inconsistent behaviors with this docstring will be fixed in subsequent PRs.

Description of the changes

Add docstrings to methods in BaseStorage class.

ytsmiling · 2020-04-30T21:30:09Z

~~Current doc and implementation conflicts with #1174 and allows to change the attributes of WAITING trials, which should be fixed.~~
#1174 does not add the restriction.

ytsmiling · 2020-04-30T21:32:29Z

optuna/storages/base.py

-        # type: (int, TrialState) -> bool
+    def set_trial_state(self, trial_id: int, state: TrialState) -> bool:
+
+        """Update a state of a specified trial.


I don't think we should allow the "RUNNING → WAITING" state change. (Both this doc and current implementation allows it now.)

This PR won't address this issue.

…ess of study_id.

optuna/storages/base.py

Co-authored-by: Takeru Ohta <phjgt308@gmail.com>

sile

LGTM!

hvy

Thanks for making this clearly documented once for all. The documented behavior LGTM. I left comments on the style and minor things. I'd personally like to change some English phrases but those are minor and sort of repeat themselves so I could follow it up in a different PR if that's fine with you.

optuna/storages/base.py

hvy · 2020-05-15T08:30:49Z

optuna/storages/base.py

+        Returns:
+            Study ID, which is an unique id among studies, of the created study.
+
+        Raises:


Greatly improves documentation👍

We (or at least I) haven't really paid much attention to documenting exceptions, but we definitely should.

optuna/storages/base.py

ytsmiling · 2020-05-15T09:48:11Z

@hvy Thank you for your review. I appreciate your follow-up PR suggestion, and I'd like to see it.

Co-authored-by: Hiroyuki Vincent Yamazaki <hiroyuki.vincent.yamazaki@gmail.com>

optuna/storages/base.py

hvy

Some tiny details.

Edit: Not sure why my review comments were separated to before and after this comment, but please also see #1175 (review)

optuna/storages/base.py

Co-authored-by: Hiroyuki Vincent Yamazaki <hiroyuki.vincent.yamazaki@gmail.com>

codecov-commenter · 2020-05-19T03:41:05Z

Codecov Report

Merging #1175 into master will increase coverage by 0.76%.
The diff coverage is 62.12%.

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
+ Coverage   91.04%   91.80%   +0.76%     
==========================================
  Files         142      149       +7     
  Lines       12255    13072     +817     
==========================================
+ Hits        11157    12001     +844     
+ Misses       1098     1071      -27

Impacted Files	Coverage Δ
optuna/storages/base.py	`68.22% <62.12%> (+11.90%)`	⬆️
optuna/testing/storage.py	`80.00% <0.00%> (-2.86%)`	⬇️
tests/storages_tests/rdb_tests/test_storage.py	`95.49% <0.00%> (-1.58%)`	⬇️
tests/pruners_tests/test_hyperband.py	`97.39% <0.00%> (-0.61%)`	⬇️
optuna/storages/rdb/models.py	`95.19% <0.00%> (-0.44%)`	⬇️
optuna/storages/rdb/storage.py	`96.06% <0.00%> (-0.16%)`	⬇️
optuna/storages/in_memory.py	`97.71% <0.00%> (-0.15%)`	⬇️
tests/test_study.py	`98.15% <0.00%> (-0.12%)`	⬇️
optuna/samplers/tpe/sampler.py	`88.21% <0.00%> (-0.10%)`	⬇️
optuna/distributions.py	`93.75% <0.00%> (-0.09%)`	⬇️
... and 41 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 0589b2b...ab77eb4. Read the comment docs.

hvy

Thanks for addressing, LGTM!

Add docstring to BaseStorage method interfaces.

979a0bf

ytsmiling mentioned this pull request Apr 28, 2020

Major refactoring of storage classes. #1170

Closed

20 tasks

hvy self-assigned this Apr 30, 2020

ytsmiling mentioned this pull request Apr 30, 2020

Increase test coverage of storage tests for single worker cases. #1191

Merged

47 tasks

ytsmiling commented Apr 30, 2020

View reviewed changes

hvy added document Documentation related. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. labels May 1, 2020

sile self-assigned this May 7, 2020

ytsmiling added 3 commits May 12, 2020 11:50

Remove redundant blank lines.

e372a3c

Add todo comment which tells that RDB storage does not ensure uniquen…

e4257c1

…ess of study_id.

Add sphinx links to python objects and exceptions.

e6d3a8f

sile reviewed May 13, 2020

View reviewed changes

optuna/storages/base.py Outdated Show resolved Hide resolved

optuna/storages/base.py Outdated Show resolved Hide resolved

optuna/storages/base.py Outdated Show resolved Hide resolved

sile added this to the v1.5.0 milestone May 13, 2020

ytsmiling and others added 3 commits May 13, 2020 15:48

Improve method doc consistency.

7e801e7

Co-authored-by: Takeru Ohta <phjgt308@gmail.com>

Remoe unnecessary #NOQA comments.

95ba062

Co-authored-by: Takeru Ohta <phjgt308@gmail.com>

Remove new method from BaseStorage.

cfd6f21

sile approved these changes May 13, 2020

View reviewed changes

toshihikoyanase mentioned this pull request May 15, 2020

Release Tasks for v1.5.0. #1245

Closed

2 tasks

hvy requested changes May 15, 2020

View reviewed changes

ytsmiling and others added 3 commits May 15, 2020 19:09

Cosmetic improvements of storage doc.

099399b

Co-authored-by: Hiroyuki Vincent Yamazaki <hiroyuki.vincent.yamazaki@gmail.com>

Fix domains, typo, missing docs in BaseStorage methods' docstring.

1e88e65

Change method definition order.

05c5913

ytsmiling requested a review from hvy May 18, 2020 00:54

hvy reviewed May 18, 2020

View reviewed changes

optuna/storages/base.py Outdated Show resolved Hide resolved

optuna/storages/base.py Outdated Show resolved Hide resolved

optuna/storages/base.py Outdated Show resolved Hide resolved

hvy requested changes May 18, 2020

View reviewed changes

optuna/storages/base.py Outdated Show resolved Hide resolved

optuna/storages/base.py Outdated Show resolved Hide resolved

Improve markups in BaseStorage doc.

ab77eb4

Co-authored-by: Hiroyuki Vincent Yamazaki <hiroyuki.vincent.yamazaki@gmail.com>

Merge branch 'upstream-master' into add-storage-method-doc

eeb3d11

hvy approved these changes May 19, 2020

View reviewed changes

hvy merged commit 9b3eccf into optuna:master May 19, 2020

hvy mentioned this pull request May 20, 2020

Cosmetic fixes to documentation in BaseStorage. #1261

Merged

Uh oh!

Conversation

ytsmiling commented Apr 28, 2020

Motivation

Description of the changes

Uh oh!

ytsmiling commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ytsmiling Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

ytsmiling May 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sile left a comment

Choose a reason for hiding this comment

Uh oh!

hvy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hvy May 15, 2020

Choose a reason for hiding this comment

Uh oh!

hvy May 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ytsmiling commented May 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hvy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 19, 2020

Codecov Report

Uh oh!

hvy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ytsmiling commented Apr 30, 2020 •

edited

Loading

hvy left a comment •

edited

Loading