Fixed incorrect colour in ErrorBar when Nan value is presented by henryhu123 · Pull Request #16724 · matplotlib/matplotlib · GitHub

PR Summary

Fixed incorrect colour in ErrorBar when Nan value is presented
See #13799
Co-author : Dennis Tismenko Dennis.Tismenko@mail.utoronto.ca

After analyzing the codebase, the inconsistency issue was traced to the difference between the handling of NaN values in Axes.scatter versus Axes.hlines/Axes.vlines. In the latter case, NaN values were being deleted from their respective np.array, whereas in the former case, NaN values were handled by using a np.ma.array (masked array), where the NaN values were simply masked.

Removed cbook.delete_masked_points() when passing X and Y to Errorbar.

PR Checklist

Has Pytest style unit tests
Code is Flake 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Overall this seems like the correct fix 👍

I am a bit concerned about the images that changed but look identical. Are there very small changes in them?

For the tests would it be possible to use check_figures_equal instead of adding new images?

See https://matplotlib.org/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal and https://matplotlib.org/devel/testing.html#writing-an-image-comparison-test

Could you please also simplify / sqaush the git history?

Overall this seems like the correct fix 👍

I am a bit concerned about the images that changed but look identical. Are there very small changes in them?

For the tests would it be possible to use check_figures_equal instead of adding new images?

See https://matplotlib.org/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal and https://matplotlib.org/devel/testing.html#writing-an-image-comparison-test

Sorry accidentally closed the PR. Actually I didn't modify the images for hlines_basics, hlines_with_nan, hlines_masked, vlines_basics, vlines_with_nan, vlines_masked. Not sure why GitHub shows it is modified, I will look into at this issue. Thank you for your feedback.

Overall this seems like the correct fix 👍
I am a bit concerned about the images that changed but look identical. Are there very small changes in them?
For the tests would it be possible to use check_figures_equal instead of adding new images?
See https://matplotlib.org/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal and https://matplotlib.org/devel/testing.html#writing-an-image-comparison-test

Sorry accidentally closed the PR. Actually I didn't modify the images for hlines_basics, hlines_with_nan, hlines_masked, vlines_basics, vlines_with_nan, vlines_masked. Not sure why GitHub shows it is modified, I will look into at this issue. Thank you for your feedback.

Re-uploaded the original test images, test images are not changed now. Can we do the squash merge?

@henryhu123 I squashed the git history, so I don't think there's a need for a squash merge.

Included changes to the tests to use check_figures_equal instead of image comparison, thank you for the good suggestion.

Please give your commits meaningful message; #13799 is meaningless in a git log.

Combine array masks rather than deleting masked points to maintain consistency across the project. Add appropriate test cases for validating color correctness for hlines and vlines. Fixes issue matplotlib#13799.

@QuLogic Thank you for your feedback, I implemented both the changes to the commit message and the code wrapping. We also implemented your suggestions for the tests and fixed the incorrect method calls.

I think you might have misunderstood what parametrization is for, and using cycler, while cool, is a bit overkill here. Almost all of your cyclers are 1 element except one that is 2, so their product is 2 long. It seems less work just to write out those 2 differing things.

The point of parametrizing is to remove duplication. Most of that code should still be in the test, and only the changing things need to be parametrized. Certainly the expected data is fixed and there's no need to obfuscate it by putting it in a parameter. I was thinking more that you should parametrize over vlines/hlines originally, though parametrizing over the two test cases is also a good idea.

I think you might have misunderstood what parametrization is for, and using cycler, while cool, is a bit overkill here. Almost all of your cyclers are 1 element except one that is 2, so their product is 2 long. It seems less work just to write out those 2 differing things.

The point of parametrizing is to remove duplication. Most of that code should still be in the test, and only the changing things need to be parametrized. Certainly the expected data is fixed and there's no need to obfuscate it by putting it in a parameter. I was thinking more that you should parametrize over vlines/hlines originally, though parametrizing over the two test cases is also a good idea.

I have removed cycler and moved expected data to test cases. Not sure if I understand your last sentence correctly, I removed duplicated code as much as possible, and created one parametrizing generator function for both hlines/vlines test cases.

You can greatily simplify the tests:

The only variable part is the data. All other parameters are identical. You can just hard-code them. No need to build dicts and pass them as kwargs.
ymin/ymax can be scalars.
You can test hlines and vlines within one figure:
Optionally / alternatively, you can make 2 or 4 subplots and draw the different cases into different axes on the same figure.

Removed passing kwargs, hard-code the identical values.
ymin/ymax are scalars now
hlines and vlines is in one figure with 2 subplots now.

Simpler and faster:

        masked_verts = np.ma.empty((len(x), 2, 2))
        masked_verts[:, 0, 0] = x
        masked_verts[:, 1, 0] = x
        masked_verts[:, 0, 1] = ymin
        masked_verts[:, 1, 1] = ymax

Yes, that is definitely cleaner than what I had before, thank you for the suggestion! Pushed.

@timhoffm Hi, do you have any idea why Travis CI MacOs build test failed, I have looked through error information and have no idea how to fix it.

This is a known issue #16849.

Pushed the suggested order changes to the creation of masked_verts.

Thanks to all who have contributed to this!

tacaswell added this to the v3.3.0 milestone Mar 11, 2020

tacaswell self-assigned this Mar 11, 2020

henryhu123 closed this Mar 11, 2020

henryhu123 reopened this Mar 11, 2020

DennisTismenko force-pushed the Fix-13799 branch from 6b22713 to 711abc9 Compare March 11, 2020 09:14

QuLogic reviewed Mar 12, 2020

View reviewed changes

Comment thread lib/matplotlib/axes/_axes.py Outdated

Comment thread lib/matplotlib/axes/_axes.py Outdated

Comment thread lib/matplotlib/tests/test_axes.py Outdated

Comment thread lib/matplotlib/tests/test_axes.py Outdated

Resolve inconsistent NaN handling behaviour

db133b0

Combine array masks rather than deleting masked points to maintain consistency across the project. Add appropriate test cases for validating color correctness for hlines and vlines. Fixes issue matplotlib#13799.

DennisTismenko force-pushed the Fix-13799 branch from 711abc9 to db133b0 Compare March 13, 2020 05:42

henryhu123 requested a review from QuLogic March 13, 2020 23:02

Fix test cases based on feedback

8f9ce77

DennisTismenko force-pushed the Fix-13799 branch from 0322cbd to 8f9ce77 Compare March 14, 2020 00:42

timhoffm reviewed Mar 19, 2020

View reviewed changes

Improved test cases

6d93d10

henryhu123 requested a review from timhoffm March 19, 2020 17:54

timhoffm reviewed Mar 20, 2020

View reviewed changes

Refactor masked vertices creation

5dc55ee

henryhu123 requested a review from timhoffm March 20, 2020 23:20

timhoffm approved these changes Mar 21, 2020

View reviewed changes

QuLogic approved these changes Mar 22, 2020

View reviewed changes

Comment thread lib/matplotlib/axes/_axes.py Outdated

Comment thread lib/matplotlib/axes/_axes.py

Refactor hlines and vlines

583d175

timhoffm merged commit 382be60 into matplotlib:master Mar 22, 2020

timhoffm mentioned this pull request Mar 22, 2020

BUG: incorrect error bar colors when NaN values are present #13799

Closed

jklymak mentioned this pull request Jul 23, 2020

Fixes incorrect error bar colors when NaN values are present #13825

Closed

6 tasks

+              @pytest.mark.parametrize('kwargs', generate_lines_with_colors_inputs())
+              @check_figures_equal(extensions=["png"])
+              def test_vlines_with_colors(fig_test, fig_ref, kwargs):

+                      ymin = np.ma.resize(ymin, x.shape)
+                      ymax = np.ma.resize(ymax, x.shape)
+                      masked_verts = [np.ma.array([xymin, xymax])

Fixed incorrect colour in ErrorBar when Nan value is presented by henryhu123 · Pull Request #16724 · matplotlib/matplotlib · GitHub

Repository navigation

Fixed incorrect colour in ErrorBar when Nan value is presented - #16724

Fixed incorrect colour in ErrorBar when Nan value is presented#16724timhoffm merged 5 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom CSCD01-team20:Fix-13799CSCD01-team20/matplotlib:Fix-13799Copy head branch name to clipboard

Conversation

henryhu123 commented Mar 11, 2020

PR Summary

PR Checklist

Uh oh!

tacaswell commented Mar 11, 2020

Uh oh!

tacaswell commented Mar 11, 2020

Uh oh!

henryhu123 commented Mar 11, 2020

Uh oh!

henryhu123 commented Mar 11, 2020

Uh oh!

henryhu123 commented Mar 11, 2020

Uh oh!

DennisTismenko commented Mar 11, 2020

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DennisTismenko commented Mar 13, 2020

Uh oh!

QuLogic commented Mar 13, 2020

Uh oh!

henryhu123 commented Mar 13, 2020

Uh oh!

timhoffm Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henryhu123 Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

timhoffm Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

DennisTismenko Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

henryhu123 commented Mar 22, 2020

Uh oh!

QuLogic commented Mar 22, 2020

Uh oh!

Uh oh!

Uh oh!

DennisTismenko commented Mar 22, 2020

Uh oh!

timhoffm commented Mar 22, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fixed incorrect colour in ErrorBar when Nan value is presented#16724
timhoffm merged 5 commits into
matplotlib:masterfrom
CSCD01-team20:Fix-13799

timhoffm Mar 19, 2020 •
edited
Loading