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

Documentation of Algorithim behind optuna #922

Closed
wants to merge 4 commits into from

Conversation

arpitkh101
Copy link
Contributor

This PR meets requirements related to issue in #835

@hvy hvy added the document Documentation related. label Feb 14, 2020
optuna/study.py Outdated Show resolved Hide resolved
@hvy hvy self-assigned this Feb 14, 2020
@hvy
Copy link
Member

hvy commented Feb 14, 2020

By the way, you can check the CI (circleci) results from the links as the bottom of this PR. There is one currently failing, if you could take a look.

@hvy hvy linked an issue Feb 14, 2020 that may be closed by this pull request
optuna/study.py Outdated Show resolved Hide resolved
@arpitkh101
Copy link
Contributor Author

arpitkh101 commented Feb 16, 2020

@hvy
Actually, earlier I didn't add that extra line which you suggested to remove but after adding I got the same error what I was getting then.
This is just beginning of my open-source contributions, that is why I am facing such issues.
I am looking towards this error problem and if possible please suggest me changes.Sorry for inconvenience caused.

@hvy
Copy link
Member

hvy commented Feb 17, 2020

The current format seem correct. Indeed you need an empty line but then there was this other error that it wasn't entirely empty but contained whitespaces.

Copy link
Member

@hvy hvy left a comment

Choose a reason for hiding this comment

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

The documentation content basically LGTM. I left some comments. It by the way seems like you've included changes from other PRs. One way to fix your PR changes would be to rebase on the latest master, i.e. first update your local copy of the master git checkout master && git fetch <name of upstream> && git merge <name of upstream>/master --ff-only and then git rebase -i master.

optuna/study.py Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 17, 2020

Codecov Report

Merging #922 into master will decrease coverage by 0.09%.
The diff coverage is 84%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #922     +/-   ##
=========================================
- Coverage   90.32%   90.22%   -0.1%     
=========================================
  Files         110      112      +2     
  Lines        9087     9293    +206     
=========================================
+ Hits         8208     8385    +177     
- Misses        879      908     +29
Impacted Files Coverage Δ
optuna/study.py 92.72% <84%> (-0.9%) ⬇️
optuna/integration/cma.py 93.95% <0%> (ø) ⬆️
optuna/_experimental.py 100% <0%> (ø) ⬆️
optuna/progress_bar.py 48.83% <0%> (ø)
optuna/storages/rdb/alembic/versions/v1.2.0.a.py 83.33% <0%> (ø)
optuna/storages/rdb/storage.py 96.87% <0%> (+0.01%) ⬆️
optuna/storages/in_memory.py 97.83% <0%> (+0.03%) ⬆️
optuna/structs.py 94.11% <0%> (+0.04%) ⬆️
optuna/samplers/tpe/sampler.py 87.83% <0%> (+0.09%) ⬆️
tests/test_trial.py 99.24% <0%> (+0.12%) ⬆️
... and 4 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 332c4f3...6550999. Read the comment docs.

@hvy
Copy link
Member

hvy commented Feb 18, 2020

I see you're having some issues with organizing the commit history. If you really can't work it out, closing this PR and creating a new one is also fine. Just so you know.

@arpitkh101 arpitkh101 closed this Feb 18, 2020
@hvy
Copy link
Member

hvy commented Feb 18, 2020

I believe this is the new PR that's replacing this one #940.

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.

Document the algorithm behind optuna
3 participants