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 isFlake 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

tacaswell added this to thev3.3.0 milestone

Mar 11, 2020

Copy link

Member

tacaswell commentedMar 11, 2020

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 usecheck_figures_equal instead of adding new images?

Seehttps://matplotlib.org/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal andhttps://matplotlib.org/devel/testing.html#writing-an-image-comparison-test

Copy link

Member

tacaswell commentedMar 11, 2020

Could you please also simplify / sqaush the git history?

tacaswell self-assigned this

Mar 11, 2020

Copy link

ContributorAuthor

henryhu123 commentedMar 11, 2020

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 usecheck_figures_equal instead of adding new images?
Seehttps://matplotlib.org/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal andhttps://matplotlib.org/devel/testing.html#writing-an-image-comparison-test

henryhu123 closed this

Mar 11, 2020

henryhu123 reopened this

Mar 11, 2020

Copy link

ContributorAuthor

henryhu123 commentedMar 11, 2020

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 usecheck_figures_equal instead of adding new images?
Seehttps://matplotlib.org/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal andhttps://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.

Copy link

ContributorAuthor

henryhu123 commentedMar 11, 2020

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 usecheck_figures_equal instead of adding new images?
Seehttps://matplotlib.org/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal andhttps://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?

DennisTismenko force-pushed theFix-13799 branch from6b22713 to711abc9Compare

March 11, 2020 09:14

Copy link

Contributor

DennisTismenko commentedMar 11, 2020

@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 usecheck_figures_equal instead of image comparison, thank you for the good suggestion.

QuLogic reviewed

Mar 12, 2020

View reviewed changes

Copy link

Member

QuLogic left a comment

Choose a reason for hiding this comment

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

Please give your commitsmeaningful message;#13799 is meaningless in agit log.

lib/matplotlib/axes/_axes.py OutdatedShow resolvedHide resolved

lib/matplotlib/tests/test_axes.py OutdatedShow resolvedHide resolved

Resolve inconsistent NaN handling behaviour

db133b0

Combine array masks rather than deleting masked points to maintainconsistency across the project.Add appropriate test cases for validating color correctness for hlinesand vlines.Fixes issuematplotlib#13799.

DennisTismenko force-pushed theFix-13799 branch from711abc9 todb133b0Compare

March 13, 2020 05:42

Copy link

Contributor

DennisTismenko commentedMar 13, 2020

@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.

Copy link

Member

QuLogic commentedMar 13, 2020

I think you might have misunderstood what parametrization is for, and usingcycler, 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 overvlines/hlines originally, though parametrizing over the two test cases is also a good idea.

Copy link

ContributorAuthor

henryhu123 commentedMar 13, 2020

I think you might have misunderstood what parametrization is for, and usingcycler, 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 overvlines/hlines originally, though parametrizing over the two test cases is also a good idea.

I have removedcycler 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.

henryhu123 requested a review fromQuLogic

March 13, 2020 23:02

Fix test cases based on feedback

8f9ce77

DennisTismenko force-pushed theFix-13799 branch from0322cbd to8f9ce77Compare

March 14, 2020 00:42

timhoffm reviewed

Mar 19, 2020

View reviewed changes

lib/matplotlib/tests/test_axes.py Outdated


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

Copy link

Member

timhoffmMar 19, 2020•
edited
Loading

Choose a reason for hiding this comment

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

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 askwargs.
ymin/ymax can be scalars.
You can testhlines andvlines within one figure:
Optionally / alternatively, you can make 2 or 4 subplots and draw the different cases into different axes on the same figure.

Copy link

ContributorAuthor

henryhu123Mar 19, 2020

Choose a reason for hiding this comment

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

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

Improved test cases

6d93d10

henryhu123 requested a review fromtimhoffm

March 19, 2020 17:54

timhoffm reviewed

Mar 20, 2020

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated

		ymin = np.ma.resize(ymin, x.shape)
		ymax = np.ma.resize(ymax, x.shape)

		masked_verts = [np.ma.array([xymin, xymax])

Copy link

Member

timhoffmMar 20, 2020

Choose a reason for hiding this comment

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

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