Skip to content

Fix stairs() tests #18579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 1, 2020
Merged

Fix stairs() tests #18579

merged 3 commits into from
Oct 1, 2020

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Sep 25, 2020

PR Summary

CC @andrzejnovak

Fixes:

  • check_figures_equal tests did not test anything because fixture figures were not used. Instead the tests created new figures, which were not compared.
  • Limited the check_figures_equal tests to png. - No need to additionally test vector backends, because this is about the logic creating the artist, not how it's drawn by a backend.
  • Failure tests must assert with pytest.raises. pytest.xfail is only for excluding broken tests.
  • Logic change: Don't draw an edge for filled StepPatch by default (_edge_default = False). This was proposed anyway in feat: StepPatch #18275 (review) / feat: StepPatch #18275 (comment). IMHO it's the right thing to do as most of our patches behave that way; in particular bar charts which are closely related to the StepPatch.

Still there are two issues:

  • test_stairs
    The actual/expected differ in the corners of the steps. - To be investigated.
    difference:
    grafik
    expected:
    grafik

  • test_stailrs_update
    The axis limit is not updated by h.set_baseline(-2). Should it be?
    actual: grafik
    expected: grafik

@timhoffm
Copy link
Member Author

Done: The slight differences come from the fact that PathPatch and Line2D have different defaults for joinstyle and capstyle (c.f. #18597). --> Workaround here: Style Line2D so that it looks like PathPatch.

Still to do: The axis limit is not updated by h.set_baseline(-2). Should it be?

@andrzejnovak
Copy link
Contributor

Whoops, I didn't realize you were fixing it up yourself. I can remove the test changes from the other PR entirely if you prefer.

The set_baseline should not update the limit manually because from what I see set_data doesn't do that in general. The limit need to be updated manually in the test.

We have to use fixed view limits because stairs() does autoscale, but
updating data does not. Otherwise the test and ref way of generating
the plot would end up with different view limits.
@timhoffm
Copy link
Member Author

I can remove the test changes from the other PR entirely if you prefer.

@andrzejnovak Thanks, I'd prefer that. Let's get the existing functionality tested correctly before adding more functionality in #18511.

@dopplershift dopplershift merged commit 3c1b27f into matplotlib:master Oct 1, 2020
@timhoffm timhoffm deleted the fix-stair-tests branch October 1, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants