Skip to content

Don't pass unused xdescent to _get_packed_offsets. #24462

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
Nov 15, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 15, 2022

Instead of passing a list of (widths, xdescents) where xdescent is unused, just pass a list of widths. This helper is private so we just need to adjust the call sites and tests with no deprecation.

This patch is preliminary work for some further cleanup on the offsetbox module (somewhat motivated by #24386).

PR Summary

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

  • 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

Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

Just to make sure: there is no obvious use case where we would like to use xdescent, but just haven't gotten around to do it?

_Params( # total=None (implicit 1)
[(.1, 0)] * 3, total=None, sep=None, expected=(1, [0, .45, .9])),
[.1] * 3, total=None, sep=None, expected=(1, [0, .45, .9])),
Copy link
Member

Choose a reason for hiding this comment

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

Optional: Write out the list, which is a little more explicit:

Suggested change
[.1] * 3, total=None, sep=None, expected=(1, [0, .45, .9])),
[.1, .1, .1], total=None, sep=None, expected=(1, [0, .45, .9])),

assert result[0] == expected[0]
assert_allclose(result[1], expected[1])


def test_get_packed_offsets_equal_total_none_sep_none():
with pytest.raises(ValueError):
_get_packed_offsets([(1, 0)] * 3, total=None, sep=None, mode='equal')
_get_packed_offsets([1] * 3, total=None, sep=None, mode='equal')
Copy link
Member

Choose a reason for hiding this comment

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

as above

Suggested change
_get_packed_offsets([1] * 3, total=None, sep=None, mode='equal')
_get_packed_offsets([1, 1, 1], total=None, sep=None, mode='equal')

@pytest.mark.parametrize('wd_list',
([(150, 1)], [(150, 1)]*3, [(0.1, 1)], [(0.1, 1)]*2))
@pytest.mark.parametrize('widths',
([150], [150]*3, [0.1], [0.1]*2))
Copy link
Member

Choose a reason for hiding this comment

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

as above

Suggested change
([150], [150]*3, [0.1], [0.1]*2))
([150], [150, 150, 150], [0.1], [0.1, 0.1]))

Instead of passing a list of (widths, xdescents) where xdescent is
unused, just pass a list of widths.  This helper is private so we just
need to adjust the call sites and tests with no deprecation.

This patch is preliminary work for some further cleanup on the offsetbox
module.
@anntzer
Copy link
Contributor Author

anntzer commented Nov 15, 2022

@oscargus Not as far as I can tell.
@timhoffm Sure, done.

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.

Anybody can merge after CI pass.

@timhoffm timhoffm added this to the v3.7.0 milestone Nov 15, 2022
@anntzer anntzer merged commit 218cdff into matplotlib:main Nov 15, 2022
@anntzer anntzer deleted the gpo branch November 15, 2022 18:13
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.

3 participants