Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow sharing locators across axises.#28429

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
anntzer wants to merge1 commit intomatplotlib:main
base:main
Choose a base branch
Loading
fromanntzer:sl

Conversation

anntzer
Copy link
Contributor

Locally changing the assigned axis seems to be the most practical solution, if not the most elegant.

See#13438 and related discussions. This PR mostly addresses that issue, although ultimately making locators (and formatters) not have an axis attribute at all (and requiring the axis to be passed as argument) is likely the cleanest long-term design. See also#13482 for an alternative ("more magical") approach.

PR summary

PR checklist

@anntzeranntzer changed the titleAllow sharing locator across axises.Allow sharing locators across axises.Jun 20, 2024
@anntzeranntzerforce-pushed thesl branch 2 times, most recently from4bee35d to7b73776CompareJune 20, 2024 13:27
Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

This is a reasonable and clever approach. It's worth pursuing, but we still should aim for removing theaxis state eventually to make to logic simpler again.


Note that the returned ticks depend on the axis assigned to the
locator. If a given locator is used across multiple axises, make sure
that
Copy link
Member

Choose a reason for hiding this comment

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

Docstring ends prematurely.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

oops, fixed.

# note: some locators return data limits, other return view limits,
# hence there is no *one* interface to call self.tick_values.
raise NotImplementedError('Derived must override')

def _call_with_axis(self, axis):
Copy link
Member

Choose a reason for hiding this comment

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

This should get a docstring. If I understand it correctly, we always want to call this method instead of__call__, which then makes the explicitself.axis unused because it is always temporarily overwritten with the passed parameter.

We might choose to make this public as the new API, so people can use it directly (users wanting to evaluate, and locator authors implementing their logic directly here). We could then later deprecate__call__ andself.axis/set_axis.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Docstring added. Agreed that this should be the future public API, but I didn't want to commit yet to anything.

Copy link
Member

@timhoffmtimhoffm left a comment

Choose a reason for hiding this comment

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

While this should mostly not affect users, I think we should not introduce this silently, but together with the announcement and plan how to migrate to axis-independent locators. (And possibly similar for formatters).

if self.axes.yaxis._minor_tick_kw["gridOn"]:
locs.extend(self.axes.yaxis.minor.locator())
locs.extend(
self.axes.yaxis.minor.locator._call_with_axis(self.axis.yaxis))
Copy link
Member

Choose a reason for hiding this comment

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

Typo?

Suggested change
self.axes.yaxis.minor.locator._call_with_axis(self.axis.yaxis))
self.axes.yaxis.minor.locator._call_with_axis(self.axes.yaxis))

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

oops, fixed

@anntzer
Copy link
ContributorAuthor

I suspect the long-term plan is that Locator.tick_values and Locator.__call__ should go away (as well as the axis attribute) and be replaced by Locator.position_ticks(axis) (name up to bikeshedding). But this feels like much larger surgery (especially if we want to do similar stuff on the formatters as well), whereas the PR here just fixes the actual underlying issue without touching the public API at all...

@timhoffm
Copy link
Member

timhoffm commentedApr 18, 2025
edited
Loading

without touching the public API at all...

It may be a bit artificial, but we're technically changing plot behavior by overriding the axis. A user could have done

ax.yaxis.get_major_locator().set_axis(ax.xaxis)

Without having an env to check right now, but I suspect this ensures that the same locations are used on both axes.

If my assumption is correct, we at least need to announce the change.

Locally changing the assigned axis seems to be the most practicalsolution, if not the most elegant.
@anntzer
Copy link
ContributorAuthor

Sure, added a note. It's actually surprisingly difficult to explain this without getting bogged down in technical details, so if you can think of any better wording that would be welcome.

@timhoffm
Copy link
Member

How about:

Tick locators now retrieve context information always from the axis they are used with. This allows to use a single locator instance on more than on axis.
This also means you cannot accidentally or intentionally assign a different axis to a locator usinglocator.set_axis().

We could add a warning tolocator.set_axis() that fires on any user-call but not on our own (temporary manipulation).

@anntzer
Copy link
ContributorAuthor

We can't add the warning at least until we provide a public API to _call_with_axis; otherwise, how can a third-party figure out where a custom, not-actually-installed locatorwould put ticks on an existing axis? (I guess they could doold_loc = axis.get_major_locator(); axis.set_major_locator(custom_loc); locs = custom_loc(); axis.set_major_formatter(old_loc) but that's much nastier thancustom_loc.set_axis(axis); custom_loc().)

(Preferably, also all relevant Formatter methods should gain an axis kwarg as well before we start deprecating stuff: Formatter.format_data (or the newer Formatter.format_ticks) and Formatter.format_data_short all have good reasons to know what axis they are being called on too, e.g. to know cursor pointing precision to determine the number of significant digits.)

@timhoffm
Copy link
Member

timhoffm commentedApr 21, 2025
edited
Loading

otherwise, how can a third-party figure out where a custom, not-actually-installed locator would put ticks on an existing axis?

Not sure I understand the use case. I believe we agree that there should not be any warnings when people just useset_locator(). There's usually no need for users to callset_axis(). But if they've done this previously any locator__call__ - including our internal ones would use the new axis. Your replacing direct internal__call__ by_call_with_axis may break these people since they may be assuming that theirset_axis is honored. That's why I want to warn onset_axis - something like "You can use this for manual __call__s but it is overridden for any internal usage.`

Is this about a hypothetical case

fig, ax = plt.subplots()loc = LogLocator()loc.set_axis(ax.xaxis)positions = loc()

Not sure if anybody needs this, but I think it's bearable for them to see the warning, or add a filter if they want to ignore the warning.


That said, I believe it's reasonable change calling behavior, and deprecateset_axis in one go because the changed calling behavoir has the above mentioned side-effect onset_axis. I think this should also be straight forwarnd for locator - we only have to decide on a name for_call_with_axis.

It would be nice, but not strictly necessary to do the same for formatters at the same time.

@anntzer
Copy link
ContributorAuthor

anntzer commentedApr 21, 2025
edited
Loading

That's why I want to warn on set_axis - something like "You can use this for manual __call__s but it is overridden for any internal usage.`

Sure, that's reasonable.

However, I looked into deprecating Locator.set_axis and just setting an internal _axis attribute (temporarily mirrored externally via an axis property), but it turns out this is not so easy, due to the existence of wrapper locators: ThetaLocator (polar) and MicrosecondLocator (dates) currently both wrap another tick locator and propagate the axis to theinternal locator by calling set_axis on that internal one. Sure, we could also change these wrapper locators to override _call_with_axis in a way to forward that temporary axis to the internal locator, but this feels like there's something more fundamentally wrong design-wise (e.g., third-parties could also want to write "wrapper locator" classes like ThetaLocator; should they be deprived of this mechanism? -- in other words, do we really want to have an escape hatch only available to builtin locators?)

I don't think this is anything unfixable, but (unless I missed something) this isn't just a mechanical change either, and may require some more thoughts (if we indeed want to do the deprecation now).

@timhoffm
Copy link
Member

timhoffm commentedApr 21, 2025
edited
Loading

You might introduce a transition path where we move away fromset_axis internally (e.g. through a separate_set_axis or and_axis attribute).This meansset_axis would only be used by external users, and if given, that axis would still take precedence over the_call_with_axis-axis. This ensures full backward-compatibility for existing users ofset_axis. At the same time, it allows sharing locators if they haven't been explicitly assigned an axis viaset_axis. - I believe the intersection between both cases is tiny. So you can have both worlds soon. You do not yet have to introduce new public API or deprecate anything.

But for simplicity, I firmly believe we have to get rid of the stateself.axis eventually. The two worlds from above can coexist reasonably well, but "locators can be shared, if they don't have an axis assigned explicitly" is still quirky, has a more complicated implementation and will lead to confusion in very rare edge cases (Why can I share a ScalarLocator, but not a ThetaLocator).

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@anntzer@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp