Skip to content

Move tf.keras integration#21

Merged
HideakiImamura merged 9 commits intooptuna:mainfrom
toshihikoyanase:move-tfkeras
May 11, 2023
Merged

Move tf.keras integration#21
HideakiImamura merged 9 commits intooptuna:mainfrom
toshihikoyanase:move-tfkeras

Conversation

@toshihikoyanase
Copy link
Copy Markdown
Member

@toshihikoyanase toshihikoyanase commented May 10, 2023

Motivation

This PR is a part of optuna/optuna#4484. It isolates the tf.keras integration.

Description of the changes

Move optuna/integration/tfkeras.py to optuna_integration/tfkeras.py

toshihikoyanase
toshihikoyanase commented May 10, 2023
View reviewed changes
docs/source/reference/index.rst
:toctree: generated/
:nosignatures:

optuna.integration.TFKerasPruningCallback
Copy link
Copy Markdown
Member Author

@toshihikoyanase toshihikoyanase May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure, but should I refer to optuna_integration.tfkeras.TFKerasPruningCallback instead?

Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura May 10, 2023

Choose a reason for hiding this comment

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

I'm not sure, but if we use optuna-integration, we must install optuna too. So, I think it is fine to use each integration module in optuna namespace.

Copy link
Copy Markdown
Member Author

@toshihikoyanase toshihikoyanase May 11, 2023

Choose a reason for hiding this comment

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

Thanks for your reply. In this PR, let us prioritize consistency, and let's discuss it separately.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 10, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@22450af). Click here to learn what that means.
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           @@
##             main     #21   +/-   ##
======================================
  Coverage        ?   0.00%           
======================================
  Files           ?      10           
  Lines           ?     531           
  Branches        ?       0           
======================================
  Hits            ?       0           
  Misses          ?     531           
  Partials        ?       0

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

@toshihikoyanase toshihikoyanase mentioned this pull request May 10, 2023
Merged
HideakiImamura
HideakiImamura reviewed May 10, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for the PR. I have several comments. PTAL.

docs/source/reference/index.rst
:toctree: generated/
:nosignatures:

optuna.integration.TFKerasPruningCallback
Copy link
Copy Markdown
Member

@HideakiImamura HideakiImamura May 10, 2023

Choose a reason for hiding this comment

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

I'm not sure, but if we use optuna-integration, we must install optuna too. So, I think it is fine to use each integration module in optuna namespace.

@HideakiImamura HideakiImamura self-assigned this May 10, 2023
@toshihikoyanase
Copy link
Copy Markdown
Member Author

toshihikoyanase commented May 11, 2023

Thank you for your careful review. Fixed the broken link and applied optuna_integration/_import.py. PTAL.

HideakiImamura
HideakiImamura approved these changes May 11, 2023
View reviewed changes
Copy link
Copy Markdown
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 ca3833f into optuna:main May 11, 2023
@HideakiImamura HideakiImamura added this to the v3.2.0 milestone May 11, 2023
@toshihikoyanase toshihikoyanase deleted the move-tfkeras branch May 11, 2023 05:44
@toshihikoyanase
Copy link
Copy Markdown
Member Author

toshihikoyanase commented May 11, 2023

Thanks!

@HideakiImamura HideakiImamura added the compatibility Change that breaks compatibility. label May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HideakiImamura HideakiImamura HideakiImamura approved these changes

Assignees

@HideakiImamura HideakiImamura

Labels

compatibility Change that breaks compatibility.

Projects

None yet

Milestone

v3.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@toshihikoyanase @codecov-commenter @HideakiImamura