8000 Avoid using `px.colors.sequential.Blues` that introduces `pandas` dependency by himkt · Pull Request #3422 · optuna/optuna · GitHub
[go: up one dir, main page]
Skip to content

Avoid using px.colors.sequential.Blues that introduces pandas dependency #3422

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 31, 2022
Merged

Avoid using px.colors.sequential.Blues that introduces pandas dependency #3422

merged 2 commits into from
Mar 31, 2022

Conversation

himkt
Copy link
Member
@himkt himkt commented Mar 31, 2022 •
edited
Loading

#3376 unifies our color scale in plotly visualization. This change is really awesome but it introduces (might unintentionally) pandas dependency. This PR uses plotly.colors.sequential.Blues instead of px.colors.sequential.Blues to remove the importing of plotly.express from Optuna.

Motivation

Remove plotly.express dependency. It also depends on pandas: plotly/plotly.py#2279

It confirms on my environment that the colormap of contour visualization is the same as other visualizations.

v2.10.0 This PR
_Users_himkt_Downloads_Untitled%20(1) html _Users_himkt_Downloads_Untitled html

@github-actions github-actions bot added the optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions. label Mar 31, 2022
@hvy
Copy link
Member
hvy commented Mar 31, 2022

Thanks for the quick PR.
Can you just fix the unit test that's failing the CI?

@hvy
Copy link
Member
hvy commented Mar 31, 2022

@not522 , @HideakiImamura do you have time to review this PR? (Else, I can jump in)

@himkt
Copy link
Member Author
himkt commented Mar 31, 2022

I found plotly.express.colors is simply alias to plotly.colors. It's enough to use plotly.colors.sequential.Blues instead of plotly.express.colors.sequential.Blues.

https://github.com/plotly/plotly.py/blob/4b5392221bd71d5fa338bfc2144a2fcf3451c177/packages/python/plotly/plotly/express/colors/__init__.py

> pip uninstall pandas

Found existing installation: pandas 1.4.1
Uninstalling pandas-1.4.1:
...
  Successfully uninstalled pandas-1.4.1

> python -m pytest tests/visualization_tests/test_*.py
=============================================================================== test session starts ================================================================================
...

tests/visualization_tests/test_contour.py .................                                                                                                                  [ 16%]
tests/visualization_tests/test_edf.py ...                                                                                                                                    [ 18%]
tests/visualization_tests/test_intermediate_plot.py .                                                                                                                        [ 19%]
tests/visualization_tests/test_optimization_history.py .........                                                                                                             [ 28%]
tests/visualization_tests/test_parallel_coordinate.py ..........                                                                                                             [ 37%]
tests/visualization_tests/test_param_importances.py ...                                                                                                                      [ 40%]
tests/visualization_tests/test_pareto_front.py .......................................................                                                                       [ 92%]
tests/visualization_tests/test_slice.py .....                                                                                                                                [ 97%]
tests/visualization_tests/test_utils.py ...
======================================================================== 106 passed, 1069 warnings in 3.97s ========================================================================

@codecov-commenter
Copy link

Codecov Report

Merging #3422 (94a600a) into master (8d3408a) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3422   +/-   ##
=======================================
  Coverage   91.81%   91.81%           
=======================================
  Files         156      156           
  Lines       12270    12270           
=======================================
  Hits        11266    11266           
  Misses       1004     1004
Impacted Files Coverage Δ
optuna/visualization/_utils.py 98.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@nzw0301
Copy link
Member
nzw0301 commented Mar 31, 2022

Ah, I didn't know that! Thank you for removing pandas dependency!!

HideakiImamura
HideakiImamura approved these changes Mar 31, 2022
View reviewed changes
Copy link
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.

Good catch! LGTM.

@himkt
Copy link
Member Author
himkt commented Mar 31, 2022

@nzw0301 If you're OK for this PR, it'd be nice to take over the assignment and merge. 🙏

nzw0301
nzw0301 approved these changes Mar 31, 2022
View reviewed changes
Copy link
Member
@nzw0301 nzw0301 left a comment 8000

Choose a reason for hiding this comment

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

LGTM. I confirmed the both blues are same:

import plotly.express as px
import plotly.colors

px.colors.sequential.Blues == plotly.colors.sequential.Blues
# True

@nzw0301 nzw0301 merged commit 7886c35 into optuna:master Mar 31, 2022
@nzw0301 nzw0301 added this to the v3.0.0-b0 milestone Mar 31, 2022
@not522 not522 removed their assignment Mar 31, 2022
@hvy hvy added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Mar 31, 2022
@hvy hvy changed the title Avoid using px.colors.sequential.Blues that introduces pandas dep Avoid using px.colors.sequential.Blues that introduces pandas dependency Mar 31, 2022
@himkt himkt deleted the plotly-colorscale branch March 31, 2022 12:00
@nzw0301 nzw0301 mentioned this pull request Aug 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nzw0301 nzw0301 nzw0301 approved these changes

@HideakiImamura HideakiImamura HideakiImamura approved these changes

Assignees

@HideakiImamura HideakiImamura

Labels
bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. optuna.visualization Related to the `optuna.visualization` submodule. This is automatically labeled by github-actions.
Projects
None yet
Milestone
v3.0.0-b0
Development

Successfully merging this pull request may close these issues.
6 participants
@himkt @hvy @codecov-commenter @nzw0301 @HideakiImamura @not522
0