Skip to content

Deprecated is_decade and is_close_to_int #22268

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 1 commit into from
Feb 1, 2022

Conversation

oscargus
Copy link
Member

PR Summary

Deprecating two functions that should be internal. Original issue (as discussed on gitter):


Digging into ticker.py and found the following concern: is_close_to_int uses a keyword argument atol (absolute tolerance?) comparing using absolute tolerance. However, it is also called from isdecade where the keyword argument rtol (relative tolerance?) is passed as absolut tolerance.

  1. It seems like relative tolerance makes more sense for is_close_to_int
  2. It seems inconsistent to pass rtol as atol (if now the keyword naming actually says something)

If I would change this, I guess the safest way is to provide an additional keyword argument to is_close_to_int, rtol=None, check if that is set and if so use that instead? For isdecade it is not so obvious as making it consistent will break the behavior.

(I originally just wanted to use isdecade in a few places...)


Outcome:

  • Deprecating is_decade and introducing _is_decade that use rtol as relative tolerance, not as absolute tolerance.
  • Deprecating is_close_to_int which is not replaced (rely on math.isclose and np.isclose).

The behavior is slightly changed, although it is hard to say that if it will actually be an issue. The tolerances seems quite arbitrarily set and considering that these are mainly used for exponents of bases, so typically rather small integers. But there may indeed be cases which will behave differently now.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus oscargus force-pushed the deprecatingtickerfunctions branch 3 times, most recently from b095bf8 to 9f781ee Compare January 19, 2022 19:41
@tacaswell tacaswell added this to the v3.6.0 milestone Jan 19, 2022
@oscargus oscargus force-pushed the deprecatingtickerfunctions branch 2 times, most recently from e31f60c to 4847ec3 Compare January 21, 2022 23:12
@oscargus
Copy link
Member Author

Too tired, so shouldn't have commented.

I think I have now sorted out the issues though.

@oscargus oscargus force-pushed the deprecatingtickerfunctions branch 3 times, most recently from 528954f to ba0d30d Compare January 21, 2022 23:19
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I left one picky comment, but otherwise this looks good!

@oscargus oscargus force-pushed the deprecatingtickerfunctions branch 2 times, most recently from e4d2aae to 42476a1 Compare January 21, 2022 23:25
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.

I would not replace the internal usages of is_close_to_int.

is_close_to_int(2 * x) is more readable than np.isclose(2 * x, round(2 * x)).

@jklymak
Copy link
Member

jklymak commented Jan 22, 2022

Agree with Tim, there are many ways to express "close to" so it is useful to wrap one way.

@oscargus
Copy link
Member Author

Alright. Should I still deprecate the original is_close_to_int and introduce a new one (supporting both rel_tol and abs_tol similar to math.isclose?).

@oscargus oscargus marked this pull request as draft January 26, 2022 17:50
@oscargus oscargus force-pushed the deprecatingtickerfunctions branch from 42476a1 to a3359f6 Compare January 30, 2022 11:22
@oscargus oscargus marked this pull request as ready for review January 30, 2022 11:24
@oscargus
Copy link
Member Author

This is now updated.

@oscargus oscargus force-pushed the deprecatingtickerfunctions branch from bf9b318 to 7d37c29 Compare January 31, 2022 09:43
@timhoffm timhoffm merged commit dc11ded into matplotlib:main Feb 1, 2022
@oscargus oscargus deleted the deprecatingtickerfunctions branch February 1, 2022 11: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