Skip to content
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

func argument in legend_elements with non-monotonically increasing functions #17546

Merged
merged 5 commits into from
Aug 4, 2020

Conversation

rlung
Copy link
Contributor

@rlung rlung commented Jun 1, 2020

Account for inverse (or other) relationships where np.interp will not work correctly.

PR Summary

legend_elements does not account for inverse or non-monotonically increasing functions. See this post for an example. The problem is in using np.interp, which requires a monotonically increasing input xp. This PR sorts xp first before calling np.interp.

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

@rlung rlung changed the title Update collections.py func argument in legend_elements with non-monotonically increasing functions Jun 1, 2020
@anntzer
Copy link
Contributor

anntzer commented Jun 1, 2020

It's not clear to me why we bother going through this interpolation. After all, immediately above we assume that func can be called with all values at once (label_values = func(values)), so why not do the same here?

@rlung
Copy link
Contributor Author

rlung commented Jun 1, 2020

I suppose because the label values are the "starting point" to ideally get somewhat even/round numbers to display. The inverse of func would be required to get the sizes from these label values so interpolation might be an easier way to get there. In the line of code you reference, the idea seems to be to just use the data points available for the legend and not necessarily even/round numbers.

@ImportanceOfBeingErnest
Copy link
Member

I'm pretty sure I just forgot that numpy.interp needs increasing input. (obviously any non-monotonic function would produce senseless output anyways)
So this looks like a correct fix.
The test suite results are pretty messy, I doubt the failures are from this PR. Yet, adding a test for the case in question (a 1/x relation or similar) would be good.

@QuLogic
Copy link
Member

QuLogic commented Jun 3, 2020

The test failures are unrelated. However, please add a test for this new case.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Modulo adding a test.

@tacaswell tacaswell added this to the v3.4.0 milestone Jun 18, 2020
@rlung
Copy link
Contributor Author

rlung commented Jun 21, 2020

Added a test. I'm fairly new to this, so I'm not sure if the test is causing some of these failures. However, there were a lot of failures initially with this PR, though I don't know how they were resolved before these recent changes.

@QuLogic
Copy link
Member

QuLogic commented Jul 22, 2020

If you rebase this, it should fix the CI failures.

Account for inverse (or other) relationships where `np.interp` will not work correctly.
@jkseppan jkseppan merged commit ede8e56 into matplotlib:master Aug 4, 2020
@jkseppan
Copy link
Member

jkseppan commented Aug 4, 2020

Thanks @rlung!

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.

None yet

7 participants