Allow passing a transformation to secondary_xaxis/_yaxis #25224

ZachDaChampion · 2023-02-15T18:18:51Z

PR Summary

resolves #25119.

I'm new to Matplotlib development so not sure if this is the best way to do this. I added a transform parameter to secondary_xaxis and secondary_yaxis and passed that through into SecondaryAxis._set_location(), where the transform is blended.

if self._orientation == 'x':
    # An x-secondary axes is like an inset axes from x = 0 to x = 1 and
    # from y = pos to y = pos + eps, in the parent's transAxes coords.
    bounds = [0, self._pos, 1., 1e-10]

    # If a transformation is provided, use its y component rather than
    # the parent's transAxes. This can be used to place axes in the data
    # coords, for instance.
    if transform is not None:
        transform = transforms.blended_transform_factory(
            self._parent.transAxes, transform)
else:  # 'y'
    bounds = [self._pos, 0, 1e-10, 1]
    if transform is not None:
        transform = transforms.blended_transform_factory(
            transform, self._parent.transAxes)  # Use provided x axis

# If no transform is provided, use the parent's transAxes
if transform is None:
    transform = self._parent.transAxes

I also updated test_secondary_xy() and test_secondary_fail() to test the transform.

I'm not entirely sure if/where I'm meant to put .. versionchanged:: in the docsting, but I'm happy to add it if it's needed.

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

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

jklymak · 2023-02-15T18:45:25Z

This looks really useful, and a quick glance the code is great. Can we add an example, at least in the issue that shows the usefulness of this? Probably want one in the example gallery as well. I had to read your code to understand the context here, which if I understand is to allow the secondary xaxis to have a y position specified in data co-ordinates rather than just axes co-ordinates.

If that is the case, what happens if the y data limits are set so the secondary axis is out of the Axes viewport?

ZachDaChampion · 2023-02-15T19:33:11Z

@jklymak Yes, you are correct on the context. Right now, if an axis is out of the viewport it isn't drawn. The existing function behaves the same if you give it a location outside of the viewport. E.g. x2 = ax.secondary_xaxis(1.5) is not drawn. If we want it to do something else I'd be happy to change it.

Good point on the examples, I'll take care of that.

melissawm · 2023-03-09T18:19:43Z

Hi @ZachDaChampion - it seems you also need a rebase here to incorporate changes made in the main branch. Here's a handy guide: https://matplotlib.org/stable/devel/development_workflow.html#rebasing-on-upstream-main

Feel free to ping if you need more guidance!

ksunden · 2023-04-04T16:36:02Z

It appears that you did a merge instead of a rebase, which results in 884 commits (!!) showing up here.

We have some docs for recovering from situations like this:

https://matplotlib.org/stable/devel/development_workflow.html#recovering-from-mess-ups

ZachDaChampion · 2023-04-04T16:40:48Z

It appears that you did a merge instead of a rebase, which results in 884 commits (!!) showing up here.

We have some docs for recovering from situations like this:

https://matplotlib.org/stable/devel/development_workflow.html#recovering-from-mess-ups

My bad, I didn't realize I pushed that version. Thanks for the link

lib/matplotlib/axes/_axes.py

lib/matplotlib/axes/_secondary_axes.py

lib/matplotlib/tests/test_axes.py

QuLogic · 2023-04-15T02:59:56Z

Now that we have typing stubs, it looks like you'll have to update the corresponding lines in the .pyi files. Also, please rebase to fix the CircleCI failures.

galleries/examples/subplots_axes_and_figures/secondary_axis.py

lib/matplotlib/axes/_secondary_axes.py

melissawm · 2023-05-24T14:30:52Z

Gentle ping to the reviewers - can you take another look here? Thanks!

QuLogic

Just a couple minor fixes.

lib/matplotlib/axes/_axes.py

lib/matplotlib/axes/_secondary_axes.py

ZachDaChampion · 2023-05-31T16:19:47Z

I can rebase later if needed

QuLogic

Sorry we took a while to get back to this again. Just a couple minor fixes, but otherwise LGTM.

QuLogic · 2023-07-22T02:26:59Z

galleries/examples/subplots_axes_and_figures/secondary_axis.py

+
+fig, ax = plt.subplots(layout='constrained')
+x = np.arange(0, 10)
+np.random.seed(34)


We almost always use 19680801 as seed

Suggested change

np.random.seed(34)

np.random.seed(19680801)

QuLogic · 2023-07-22T03:10:33Z

lib/matplotlib/axes/_axes.py

-        else:
-            raise ValueError('secondary_yaxis location must be either '
+        if not (location in ['left', 'right'] or isinstance(location, Real)):
+            raise ValueError('secondary_xaxis location must be either '


Typo:

Suggested change

raise ValueError('secondary_xaxis location must be either '

raise ValueError('secondary_yaxis location must be either '

QuLogic · 2023-07-22T03:11:40Z

lib/matplotlib/axes/_secondary_axes.py

+        # Make sure transform is a Transform or None.
+        _api.check_isinstance((transforms.Transform, None), transform=transform)


I wouldn't bother with the comment; the function call is clear:

Suggested change

# Make sure transform is a Transform or None.

_api.check_isinstance((transforms.Transform, None), transform=transform)

_api.check_isinstance((transforms.Transform, None), transform=transform)

Add transform argument to secondary axes Update _secax_docstring Move new params to end of functions Add input check to secondary axes Add tests Add examples Move transform type checks and improve docs Add type stubs Update _secax_docstring Move new params to end of functions Add input check to secondary axes Move transform type checks and improve docs Fix rebase error Fix stub for SecondaryAxis.__init__ Clarify example Add default param to secax constructor Fix stub for secax constructor Simplify imports Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> Remove redundancy in docs Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> Fix typo

QuLogic · 2024-02-23T02:30:02Z

I've rebased, squashed into one commit, fixed the typos, and force pushed here in the interest of getting this merged.

lib/matplotlib/axes/_axes.py

timhoffm · 2024-03-08T10:39:05Z

Thanks @ZachDaChampion and congratulations on your first contribution to Matplotlib. 🎉 We'd love to see you back!

github-actions bot reviewed Feb 15, 2023

View reviewed changes

jklymak changed the title ~~Allow passing a transformation to a secondary plot~~ Allow passing a transformation to secondary_xaxis/_yaxis Feb 15, 2023

jklymak added status: needs documentation topic: axes labels Feb 16, 2023

jklymak added this to the v3.8.0 milestone Feb 16, 2023

github-actions bot added the status: needs rebase label Feb 24, 2023

github-actions bot removed the status: needs rebase label Apr 4, 2023

ZachDaChampion force-pushed the secondary-axes-trans-arg branch from ffb9b8e to cb55d34 Compare April 4, 2023 16:47

QuLogic reviewed Apr 12, 2023

View reviewed changes

ZachDaChampion force-pushed the secondary-axes-trans-arg branch from 4af5bc1 to e33828f Compare April 15, 2023 03:40

QuLogic mentioned this pull request Apr 15, 2023

[Bug]: Secondary axis does not support Transform as functions #25691

Closed

QuLogic reviewed Apr 15, 2023

View reviewed changes

galleries/examples/subplots_axes_and_figures/secondary_axis.py Outdated Show resolved Hide resolved

ksunden reviewed Apr 17, 2023

View reviewed changes

lib/matplotlib/axes/_secondary_axes.py Outdated Show resolved Hide resolved

ZachDaChampion force-pushed the secondary-axes-trans-arg branch from b2d2bc7 to 941f455 Compare April 20, 2023 21:00

QuLogic reviewed May 27, 2023

View reviewed changes

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved

lib/matplotlib/axes/_secondary_axes.py Outdated Show resolved Hide resolved

lib/matplotlib/axes/_secondary_axes.py Outdated Show resolved Hide resolved

QuLogic approved these changes Jul 22, 2023

View reviewed changes

QuLogic removed the status: needs documentation label Jul 22, 2023

ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023

github-actions bot added the status: needs rebase label Dec 12, 2023

QuLogic force-pushed the secondary-axes-trans-arg branch from 4905246 to c1b51aa Compare February 23, 2024 02:29

github-actions bot added Documentation: examples files in galleries/examples and removed topic: axes status: needs rebase labels Feb 23, 2024

QuLogic added the topic: axes label Feb 23, 2024

timhoffm reviewed Feb 23, 2024

View reviewed changes

lib/matplotlib/axes/_axes.py Show resolved Hide resolved

timhoffm reviewed Feb 23, 2024

View reviewed changes

lib/matplotlib/axes/_axes.py Show resolved Hide resolved

timhoffm approved these changes Mar 8, 2024

View reviewed changes

timhoffm merged commit 5abcae2 into matplotlib:main Mar 8, 2024
42 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing a transformation to secondary_xaxis/_yaxis #25224

Allow passing a transformation to secondary_xaxis/_yaxis #25224

ZachDaChampion commented Feb 15, 2023 •

edited

github-actions bot left a comment

jklymak commented Feb 15, 2023

ZachDaChampion commented Feb 15, 2023

melissawm commented Mar 9, 2023

ksunden commented Apr 4, 2023

ZachDaChampion commented Apr 4, 2023

QuLogic commented Apr 15, 2023

melissawm commented May 24, 2023

QuLogic left a comment

ZachDaChampion commented May 31, 2023

QuLogic left a comment

QuLogic Jul 22, 2023

QuLogic Jul 22, 2023

QuLogic Jul 22, 2023

QuLogic commented Feb 23, 2024

timhoffm commented Mar 8, 2024

	raise ValueError('secondary_xaxis location must be either '
	raise ValueError('secondary_yaxis location must be either '

		# Make sure transform is a Transform or None.
		_api.check_isinstance((transforms.Transform, None), transform=transform)

Allow passing a transformation to secondary_xaxis/_yaxis #25224

Allow passing a transformation to secondary_xaxis/_yaxis #25224

Conversation

ZachDaChampion commented Feb 15, 2023 • edited

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

jklymak commented Feb 15, 2023

ZachDaChampion commented Feb 15, 2023

melissawm commented Mar 9, 2023

ksunden commented Apr 4, 2023

ZachDaChampion commented Apr 4, 2023

QuLogic commented Apr 15, 2023

melissawm commented May 24, 2023

QuLogic left a comment

Choose a reason for hiding this comment

ZachDaChampion commented May 31, 2023

QuLogic left a comment

Choose a reason for hiding this comment

QuLogic Jul 22, 2023

Choose a reason for hiding this comment

QuLogic Jul 22, 2023

Choose a reason for hiding this comment

QuLogic Jul 22, 2023

Choose a reason for hiding this comment

QuLogic commented Feb 23, 2024

timhoffm commented Mar 8, 2024

ZachDaChampion commented Feb 15, 2023 •

edited