Hi@ebubekir-pulat I see you now have three draft PRs open. Do you need some help from us to get them ready for review? In general draft PRs tend to be ignored and assumed "work in progress" unless you tell us otherwise.

Thanks for your contributions!

Thank you@rcomer for the response. Yes, I would like some advice on how to improve my pull requests to get them ready for merging. What should I write in my pull requests to clearly show I would like feedback, criticism etc?

@ebubekir-pulat we have some guidelines here to work through, but as it says, it doesn't have to be perfect. If you are stuck on something specific, do ask. If not, mark the PR ready for review when you have got it as ready as you can.

I think this fix is good to go. Is there need for any changes to the documentation?

I think some changes to eventplot-related tests in test_axes.py, and the 'test_get_font_names' test in test_font_manager.py, might need to be made.

@ebubekir-pulat have a look at thetriage_tests.py script to help you review those image test failures and replace the reference images if appropriate.

The failure for test_get_font_names may just be a missing font on your system (I also get this failure locally) so I suggest to ignore it unless it also shows in the test output below.

@rcomer Here are the results we got for the 3 failing image tests in test_axes.py.

eventplot.png

eventplot-expected.png

eventplot-failed-diff.png

test_eventplot_defaults.png

test_eventplot_defaults-expected.png

test_eventplot_defaults-failed-diff.png

test_eventplot_problem_kwargs.png

test_eventplot_problem_kwargs-expected.png

test_eventplot_problem_kwargs-failed-diff.png

Is there a certain range of RMS in the image test failures that is acceptable, so if it is within that, we can simply replace the reference images?

Can you please also show the resulting images? Most likely the scaling has changed a bit for some reason, but hard to say without seeing the new images (the expected ones are in the repo, so while it helps to have them visible for reference, the new ones are more relevant).

And after reading the code, no, the reason is that snapping is off. As@jklymak thought that snapping can be off here, it should be good to go. But please add the new images so that one see the actual effect of this.

@oscargus Would the 'new image' for test_eventplot be 'eventplot.png' for example? If so, I have accordingly updated the above comment showing the image outputs

That should be the correct ones, yes!

If you copy those to thelib/matplotlib/tests/baseline_images (plus correct subdirectorytest_YYY) and add them to the PR (overwrite the old ones), it should make it even simpler to compare as there are some tools for that in GitHub.

To me it seems like the new results are OK. In some situations the lines are bit blurred in the new one due to lack of snapping, but I think one can live with that. One should maybe add a note that the default has changed, but it can be discussed where that should be added. I'll ping@timhoffm as the API lead to have a say.

Edit: to summarize for Tim. Ifsnap is not provided foreventplot, it will now default toFalse (and antialiasing will be turned on). There is a warning, but that should maybe be rephrased as well.