Docstring overhaul #457

till-m · 2024-01-24T21:40:48Z

Rewrites most docstrings
Uses numpydoc
Include README.md in the docs
Change links in README.md to link preferrably to docs webpage
Change github links in README to link to the organization repo
Replaces colors with colorama Replace custom colour implementation with colorama colours #434
Migrate images from examples/ to static/

….py` (bayesian-optimization#435) * Replace custom colour implementation, add docs for `logger.py`, `util.py` * minor typo/syntax fixes * User `or` to separate different possible types

…an-optimization#440) * Run tests on any PR * Update docs, linting * Update bayes_opt/constraint.py Co-authored-by: Leandro Braga <[email protected]> * Rename mislabelled parameters --------- Co-authored-by: Leandro Braga <[email protected]>

…-optimization#445) * Fixes issue-436: Constrained optimization does not allow duplicate points (bayesian-optimization#437) * Update docs of `bayesian_optimization.py` and `observer.py`. * Fix minor style issue in module docstring * Update docs of `__init__.py` and `events.py`. * Fix minor style issue in class docstring * Add workflow to check docstrings * Update bayes_opt/bayesian_optimization.py Co-authored-by: Leandro Braga <[email protected]> --------- Co-authored-by: YoungJae Bae <[email protected]> Co-authored-by: Leandro Braga <[email protected]>

* Improve acq_max seeding of L-BFGS-B optimization (bayesian-optimization#297) --------- Co-authored-by: ptapping <[email protected]>

* Fixes issue-436: Constrained optimization does not allow duplicate points (bayesian-optimization#437) * Update docs of `bayesian_optimization.py` and `observer.py`. * Fix minor style issue in module docstring * Update docs of `__init__.py` and `events.py`. * Fix minor style issue in class docstring * Add workflow to check docstrings * Update bayes_opt/bayesian_optimization.py Co-authored-by: Leandro Braga <[email protected]> * Improve acq_max seeding of L-BFGS-B optimization (bayesian-optimization#297) * bounds_transformer could bypass global_bounds due to the test logic within _trim function in domain_reduction.py (bayesian-optimization#441) * Update trim bounds in domain_reduction.py Previously, when the new upper limit was less than the original lower limit, the new_bounds could bypass the global_bounds. * Update test_seq_domain_red.py Added test cases to catch an error when both bounds of new_bounds exceeded the global_bounds * Update domain_reduction.py _trim function now avoids an error when both bounds for a given parameter in new_bounds exceed the global_bounds * Update domain_reduction.py comments * fixed English in domain_reduction.py * use numpy to sort bounds, boundary exceeded warn. * simple sort test added * domain_red windows target_space to global_bounds Added windowing function to improve the convergence of optimizers that use domain_reduction. Improved comments and documentation. * target_space.max respects bounds; SDRT warnings * Remove unused function. This function was used to prototype a solution. It should not have been pushed and can be removed. * Updated target_space.py docstrings * Update tests/test_target_space.py Co-authored-by: till-m <[email protected]> * Added pbound warnings, updated various tests. * updated line spacing for consistency and style * added pbound test condition --------- Co-authored-by: till-m <[email protected]> * DomainReduction docs, docstyle * Add missing doc dependency --------- Co-authored-by: YoungJae Bae <[email protected]> Co-authored-by: Leandro Braga <[email protected]> Co-authored-by: ptapping <[email protected]> Co-authored-by: Edgar <[email protected]>

till-m · 2024-01-24T21:43:51Z

It looks like something was messed up in all the merges. I will close this PR for now and have a look when I'm back from* vacation.

codecov · 2024-01-31T21:25:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (129caac) 98.47% compared to head (17f1e27) 98.70%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
+ Coverage   98.47%   98.70%   +0.22%     
==========================================
  Files           8        8              
  Lines         590      540      -50     
  Branches       90       90              
==========================================
- Hits          581      533      -48     
  Misses          3        3              
+ Partials        6        4       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

till-m · 2024-01-31T21:32:20Z

looking back now, I think making a big PR on a seperate branch was not a great idea...

Some of the docstring rules imposed by the numpydocs convention are a bit silly imo, but I guess there is an argument to be made for consistency.
Either way, this should hopefully give us a solid base of docstrings to work with. I think we can then refine how the documentation should look in the future.

perezed00

So far everything I've seen looks good. I took the closest look at domain reduction and target space changes.

bayes_opt/domain_reduction.py

bayes_opt/target_space.py

perezed00 · 2024-02-01T19:21:31Z

@till-m please re-request my review. My previous approval of all commits was in error.

.github/workflows/build_docs.yml

.github/workflows/check_docstrings.yml

bwheelz36 · 2024-02-06T23:00:48Z

Nice work @till-m
I won't review all docstrings, but I see you implemented an automatic checker which I think is absolutely the right approach.

I'm not familiar with numpy-docs - does it allow for configuration to ignore trivial errors?
Will it detect missing docstrings/parameter descriptions in public methods? this is the main thing I'd really like to make sure we nail down.
What I have done on other code is use flake8-docstrings and then set it to ignore the largely spurious errors it throws up (things like missing spaces, full stops etc.)

till-m · 2024-02-07T10:03:07Z

Hey @bwheelz36,

I won't review all docstrings

completely understandable. If possible, could you review the changes to the README?

I'm not familiar with numpy-docs

I think we don't actually need this to check for convention. I will remove it.

Will it detect missing docstrings/parameter descriptions in public methods? this is the main thing I'd really like to make sure we nail down.

It will detect missing docstrings as-is

bayes_opt/bayesian_optimization.py:369 in public method `set_bounds`:
        D102: Missing docstring in public method

I didn't realize, but missing parameters needs to be explicitly enabled. I will enable it. Note that this does not detect missing docstrings of class documentation, or missing parameters in class documentation. This is due to the way __init__ is handled.

does it allow for configuration to ignore trivial errors?

You can additionally ignore errors by passing --add-ignore <error code> in the call. Unfortunately one cannot ignore individual lines (similar to noqa). The tool you sent is actually a wrapper around pydocstyle, so it behaves the same (except it additionally produces a number of linting issues). If we want to additionally add linting rules, we could also do that, though in that case I would suggest to use ruff which has the advantage of being both very comprehensive and very fast. However, if we want to use ruff we should merge #458 into this branch, as it requires the pyproject.toml.

bayes_opt/target_space.py

perezed00 · 2024-02-12T19:45:43Z

I took a second look at revisions in domain reduction, target space files, and several other .py files. However, I haven't looked at util.py, logger.py, or constraint.py

Within the files I checked, the suggestions I made were all rather small and generally things look good. Great work on this.

Co-authored-by: Edgar <[email protected]>

bwheelz36 · 2024-02-13T23:38:27Z

@till-m - ruff looks really cool. I'm keen to try it out in some projects - but agree right now is not the time or place for this one :-)

till-m · 2024-02-20T08:21:51Z

From my side, this PR is mostly good to go, except for a review of the README changes. Does anyone else have any other hold-ups?

README.md

bwheelz36 · 2024-02-21T06:04:51Z

README.md

@@ -1,16 +1,18 @@
 <div align="center">
-  <img src="https://github.com/fmfn/BayesianOptimization/blob/master/examples/func.png"><br><br>
+  <img src="https://raw.githubusercontent.com/bayesian-optimization/BayesianOptimization/master/static/func.png"><br><br>


if I look at your branch, it appears this image is missing now? can you double check this link still works?

Should work after the PR is merged. This link (when pointing to my repo/branch) works: https://raw.githubusercontent.com/till-m/BayesianOptimization/docstring-overhaul/static/func.png

bwheelz36 · 2024-02-21T06:05:01Z

From my side, this PR is mostly good to go, except for a review of the README changes. Does anyone else have any other hold-ups?

Just took a look at readme - made a few minor suggestions, but looks great to me!

till-m · 2024-02-21T08:56:20Z

If someone would still like to have a look, please let me know. Otherwise I will merge this early next week.

till-m · 2024-02-26T07:59:52Z

Thanks everyone for the help! Glad to see this is done -- even if it isn't perfect right now, we can work to make it better in the future! :)

@bwheelz36 looks like the image renders correctly: link

till-m and others added 7 commits August 5, 2023 16:10

Replace custom colour implementation, add docs for logger.py, `util…

201c6b2

….py` (bayesian-optimization#435) * Replace custom colour implementation, add docs for `logger.py`, `util.py` * minor typo/syntax fixes * User `or` to separate different possible types

Pydocstyle (bayesian-optimization#453)

a4df58c

* Improve acq_max seeding of L-BFGS-B optimization (bayesian-optimization#297) --------- Co-authored-by: ptapping <[email protected]>

Merge branch 'master' into docstring-overhaul

90fa99a

Merge branch 'master' into docstring-overhaul

55ffaf5

till-m closed this Jan 24, 2024

till-m reopened this Jan 31, 2024

Small fixes, minor cosmetic changes

8ffdb4f

till-m force-pushed the docstring-overhaul branch from d666e93 to 8ffdb4f Compare January 31, 2024 21:25

till-m requested review from fmfn, leandrobbraga, perezed00 and bwheelz36 January 31, 2024 21:32

till-m mentioned this pull request Jan 31, 2024

html docs: please review and suggest! #425

Closed

3 tasks

Add some more docs to target space and constraint, cosmetic changes

9dfffb7

perezed00 approved these changes Feb 1, 2024

View reviewed changes

perezed00 reviewed Feb 1, 2024

View reviewed changes

bayes_opt/domain_reduction.py Show resolved Hide resolved

perezed00 reviewed Feb 1, 2024

View reviewed changes

bayes_opt/target_space.py Outdated Show resolved Hide resolved

perezed00 reviewed Feb 1, 2024

View reviewed changes

bayes_opt/target_space.py Outdated Show resolved Hide resolved

Remove duplicate code snippet

cb7e406

till-m requested a review from perezed00 February 1, 2024 20:44

bwheelz36 reviewed Feb 6, 2024

View reviewed changes

.github/workflows/build_docs.yml Outdated Show resolved Hide resolved

bwheelz36 reviewed Feb 6, 2024

View reviewed changes

.github/workflows/check_docstrings.yml Show resolved Hide resolved

Remove numpydoc + adjust "*" formatting accordingly

cbf6c24

till-m added 2 commits February 7, 2024 16:11

Explicitly add D417, adjust code accordingly

8a0265c

Adjust TargetSpace.probe() behaviour to be in line with docstring.

170e84c

perezed00 reviewed Feb 12, 2024

View reviewed changes

bayes_opt/target_space.py Outdated Show resolved Hide resolved

Update bayes_opt/target_space.py

58abfb6

Co-authored-by: Edgar <[email protected]>

bwheelz36 reviewed Feb 21, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

bwheelz36 reviewed Feb 21, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

bwheelz36 reviewed Feb 21, 2024

View reviewed changes

Update README.md

17f1e27

till-m merged commit 383cb29 into bayesian-optimization:master Feb 26, 2024
8 checks passed

till-m mentioned this pull request Feb 26, 2024

Replace custom colour implementation with colorama colours #434

Closed

1 task

till-m deleted the docstring-overhaul branch July 20, 2024 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docstring overhaul #457

Docstring overhaul #457

till-m commented Jan 24, 2024

till-m commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

till-m commented Jan 31, 2024

perezed00 left a comment •

edited

Loading

perezed00 commented Feb 1, 2024

bwheelz36 commented Feb 6, 2024

till-m commented Feb 7, 2024 •

edited

Loading

perezed00 commented Feb 12, 2024

bwheelz36 commented Feb 13, 2024

till-m commented Feb 20, 2024

bwheelz36 Feb 21, 2024

till-m Feb 21, 2024

bwheelz36 commented Feb 21, 2024

till-m commented Feb 21, 2024

till-m commented Feb 26, 2024

Docstring overhaul #457

Docstring overhaul #457

Conversation

till-m commented Jan 24, 2024

till-m commented Jan 24, 2024 • edited Loading

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

till-m commented Jan 31, 2024

perezed00 left a comment • edited Loading

Choose a reason for hiding this comment

perezed00 commented Feb 1, 2024

bwheelz36 commented Feb 6, 2024

till-m commented Feb 7, 2024 • edited Loading

perezed00 commented Feb 12, 2024

bwheelz36 commented Feb 13, 2024

till-m commented Feb 20, 2024

bwheelz36 Feb 21, 2024

Choose a reason for hiding this comment

till-m Feb 21, 2024

Choose a reason for hiding this comment

bwheelz36 commented Feb 21, 2024

till-m commented Feb 21, 2024

till-m commented Feb 26, 2024

till-m commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

perezed00 left a comment •

edited

Loading

till-m commented Feb 7, 2024 •

edited

Loading