Skip to content

Add specific test cases for visualization.matplotlib.plot_intermediate_values#2754

Merged
nzw0301 merged 1 commit intooptuna:masterfrom
asquare100:add-test-cases-matplotlib-intermediate-plot
Jul 14, 2021
Merged

Add specific test cases for visualization.matplotlib.plot_intermediate_values#2754
nzw0301 merged 1 commit intooptuna:masterfrom
asquare100:add-test-cases-matplotlib-intermediate-plot

Conversation

@asquare100
Copy link
Copy Markdown
Contributor

Motivation

Currently, test cases in optuna/visualization/matplotlib/*.py are not enough compared to optuna/visualization/*.py.

Fixes

Issue

Description of the changes

In this PR, I have added some specific test cases to the file tests/visualization_tests/matplotlib/test_intermediate_plot.py.
I will be creating another PR to add test cases in other files of tests/visualization_tests/matplotlib/

@nzw0301 nzw0301 added the test Unit test. label Jun 21, 2021
@Crissman
Copy link
Copy Markdown
Contributor

@keisuke-umezawa Can you take a look at this?

Copy link
Copy Markdown
Contributor

@Crissman Crissman left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! LGTM.

Copy link
Copy Markdown
Member

@nzw0301 nzw0301 left a comment

Choose a reason for hiding this comment

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

I'm not in assignees, but let me leave a comment because I have reviewed similar pull requests recently.

Could you replace assert not figure.has_data() with assert len(figure.get_lines()) == 0?

@keisuke-umezawa
Copy link
Copy Markdown
Member

cc: @asquare100

Copy link
Copy Markdown
Member

@nzw0301 nzw0301 left a comment

Choose a reason for hiding this comment

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

Basically, the change looks great to me except for my comment. Since we do not get an update, let me merge this PR. I'll send a follow-up PR later.

@nzw0301 nzw0301 changed the title Added specific test cases Add specific test cases for visualization.matplotlib.plot_intermediate_values Jul 14, 2021
@nzw0301
Copy link
Copy Markdown
Member

nzw0301 commented Jul 14, 2021

Let me rename the PR title with a more informative one.

@nzw0301 nzw0301 merged commit 8c31e21 into optuna:master Jul 14, 2021
@asquare100 asquare100 deleted the add-test-cases-matplotlib-intermediate-plot branch July 14, 2021 11:30
@hvy hvy added this to the v2.9.0 milestone Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Unit test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants