Skip to content

Add a note that Locators must return ticks above and below vlims #25173

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Feb 7, 2023

Closes #25146

This was made as a demo in the new contributors meeting.
Happy to update wording/add addtional context if we want.

PR Summary

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@ksunden ksunden marked this pull request as draft February 7, 2023 19:01
@ksunden ksunden requested a review from story645 February 7, 2023 19:02
@ksunden ksunden marked this pull request as ready for review February 7, 2023 19:03
@ksunden ksunden marked this pull request as draft February 7, 2023 19:03
@dstansby
Copy link
Member

dstansby commented Feb 8, 2023

I think it's worth adding one sentence of context here. I tried writing a recommendation, but drew a blank... if I get a bit more time I'll try and dive through the archives of discussing this issue to work out a good sentence.

@dstansby
Copy link
Member

dstansby commented Feb 8, 2023

We should also check that all our locators actually adhere to this before documenting it! I've been having a play around with hypothesis to test this, and have found that for MaxNLocator with vmin=0, vmax=1, the smallest tick returned is 0 which is not < vmin...

Comment on lines +21 to +22
Locators must return at least a single tick below ``vmin`` and a single tick
above ``vmax``.
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should be changed to >=/<=, cf #25146 (comment)

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

did we ever figure out if its [] or ()?

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.

[Doc]: Document how many out of bounds ticks Locators should return
4 participants