Fixed incorrect colour in ErrorBar when Nan value is presented #16724
Conversation
|
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 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? |
|
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? |
6b22713 to
711abc9
Compare
|
@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 |
There was a problem hiding this comment.
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.
711abc9 to
db133b0
Compare
|
@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 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 |
I have removed |
0322cbd to
8f9ce77
Compare
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): |
There was a problem hiding this comment.
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
hlinesandvlineswithin one figure:
- Optionally / alternatively, you can make 2 or 4 subplots and draw the different cases into different axes on the same figure.
There was a problem hiding this comment.
- Removed passing kwargs, hard-code the identical values.
- ymin/ymax are scalars now
- hlines and vlines is in one figure with 2 subplots now.
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]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
|
Thanks to all who have contributed to this! |
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