-
Notifications
You must be signed in to change notification settings - Fork 23
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
Move integration/allennlp
#8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Almost, LGTM. Please check one minor comment.
optuna.integration.AllenNLPExecutor | ||
optuna.integration.allennlp.dump_best_config | ||
optuna.integration.AllenNLPPruningCallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These modules are listed inside the Chainer
section. I think we should create the AllenNLP
section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost LGTM! But I have a minor comment.
README.md
Outdated
* [Chainer](https://optuna.readthedocs.io/en/stable/reference/integration.html#chainer) ([example](https://github.com/optuna/optuna-examples/tree/main/chainer/chainer_integration.py)) | ||
* [ChainerMN](https://optuna.readthedocs.io/en/stable/reference/integration.html#chainermn) ([example](https://github.com/optuna/optuna-examples/tree/main/chainer/chainermn_simple.py)) | ||
* [Keras](https://optuna.readthedocs.io/en/stable/reference/integration.html#keras) ([example](https://github.com/optuna/optuna-examples/tree/main/keras)) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit trivial, but please remove this unnecessary new line.
Thank you for your review. I updated the changes accordingly. |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #8 +/- ##
=======================================
Coverage ? 76.43%
=======================================
Files ? 9
Lines ? 505
Branches ? 0
=======================================
Hits ? 386
Misses ? 119
Partials ? 0 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This reverts commit b6c50ee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Motivation
Move
integration/allennlp
from optuna/optunaDescription of the changes
Move
integration/allennlp
from optuna/optuna