Skip to content

Improve less_simple_linear_interpolation. #9859

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

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 26, 2017

  • Rewrite less_simple_linear_interpolation to use np.interp and
    np.polyfit (for the extrapolation part), document it properly and note
    the differences with np.interp.

  • Deprecate extrapolation-less mode (which is well covered by
    np.interp).

  • Simplify its usage in contour: only one place needs extrapolation, so
    the others can use np.interp; -1 is a perfectly reasonable marker for
    out of bounds which avoids having to dance with the errstate.

Conflicts with #9151 but the rebase should be easy in either direction, I'm fine with letting #9151 go in first.

Edit: Likely obsoleted by #9865.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

- Rewrite less_simple_linear_interpolation to use np.interp and
np.polyfit (for the extrapolation part), document it properly and note
the differences with np.interp.

- Deprecate extrapolation-less mode (which is well covered by
np.interp).

- Simplify its usage in contour: only one place needs extrapolation, so
the others can use np.interp; -1 is a perfectly reasonable marker for
out of bounds which avoids having to dance with the errstate.
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I haven't reviewed everything in detail, but it looks like you have some nice cleanups in a difficult chunk of code. I have a question about the reimplementation of the horribly-named less_simple_linear_interpolation.

yi[ii] = y[0]
elif xx > x[-1]:
if extrap:
yi[ii] = y[-1]
Copy link
Member

Choose a reason for hiding this comment

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

The original is using the same extrapolation method as np.interp uses by default: setting the past-the-end values to the end values of the input array. Are you changing the behavior because what mpl actually needs is linear extrapolation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I actually missed that point because my understanding of the contour code is that it indeed needs extrapolation (but I need to look at it again and run some examples). If we don't actually need extrapolation then we can definitely get rid of this function in favor of using interp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I convinced myself that linear extrapolation was not needed because the call to calc_label_rot_and_inline is protected by print_label which is explicitly there to check that the contour line is long enough to host the label text, so we don't expect overflow anyways (so flat extrapolation is as good as anything).

Rewriting in terms of np.interp...

@anntzer
Copy link
Contributor Author

anntzer commented Dec 3, 2017

Obsoleted by #9865.

@anntzer anntzer closed this Dec 3, 2017
@anntzer anntzer deleted the linear-interp branch December 3, 2017 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants