Mv dynamic function to the subplotgrid #281

cvanelteren · 2025-06-21T10:27:16Z

In order to support making RC params thread safe we need to ensure that our dynamically defined methods or classes are unified under init and minimized where necessary. This is the firs in a sequence of refactors that moves some functions to be more appropriately defined which also aids in debugging and provides more clarity.

beckermr

This pr changes the return type of the added commands. Is that intentional?

codecov · 2025-06-21T10:52:49Z

Codecov Report

❌ Patch coverage is 80.39216% with 20 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
ultraplot/gridspec.py	67.74%	10 Missing and 10 partials ⚠️

📢 Thoughts on this report? Let us know!

cvanelteren · 2025-06-21T10:59:03Z

Return types are the same I believe.

cvanelteren · 2025-06-21T11:02:36Z

Unless I am missing something -- what makes you think the return types are different? The return statement here is just whatever the wrapper returns. I used the wrapper otherwise the logic is the same everywhere.

cvanelteren · 2025-06-21T11:07:50Z

Tests pass, will need to add some tests now. Not sure if I have the time today to do this.
Will also add a comment on the return type as it may cause confusion to others.

Ah you're right. Will review again when you are ready.

cvanelteren · 2025-06-22T10:11:25Z

Not that happy with the tests, but I am also no that keen on adding some of the methods for this PR as the focus is on replacing the dynamic content. So for now it's good enough. Open to review @beckermr

cvanelteren · 2025-06-23T07:05:59Z

Will change the decorator to apply_to_all

beckermr

We need to fix the handling of the doc strings etc.

ultraplot/gridspec.py

beckermr · 2025-06-23T17:16:54Z

ultraplot/gridspec.py

-        # Apply the method
-        _grid_command.__qualname__ = f"SubplotGrid.{name}"
-        _grid_command.__name__ = name
-        _grid_command.__doc__ = doc
-        setattr(cls, name, _grid_command)


Because these lines are no longer executed, the new commands won't have correct doc strings, names, etc. This should be fixed.

functool wraps should take care of this

beckermr · 2025-06-23T17:21:26Z

ultraplot/tests/test_gridspec.py

+    """
+    fig, axs = uplt.subplots(nrows=1, ncols=2)
+    doc = axs.altx.__doc__
+    assert "Call `altx()` for every axes in the grid" in doc


The doc string should match the original docstring?

axs here is a SubplotSpec. The intent of the call is to assert t hat the decorator is correctly wrapping the docs. Unless you mean something else that I am missing (in case that is the case apologies ;-)!)

I meant to say that the doc string for axs.altx should match the doc string for ax.altx. This applies to my main comment above as well.

I'm not sure I fully agree here. The SubplotGrid is a distinct object, and the current docstring reflects its actual behavior: it applies altx to each Axis it contains. The method altx itself belongs to an Axis, not to the GridSpec or the SubplotGrid directly.

I would however, agree if we provide a link to those methods

~~Sure a link would be fine.~~

Edit: See my request below. I really don't think it is a good idea to replace a helpful doc string with one that has very little information in it.

Co-authored-by: Matthew R. Becker <[email protected]>

beckermr

Currently ultraplot does copy over the doc string

In [1]: import ultraplot as uplt

In [2]: fig, axs = uplt.subplots()

In [3]: axs.altx?
Signature: axs.altx(*args, **kwargs)
Docstring:
Add an axes locked to the same location with a
distinct x axis for every axes in the grid.
This is an alias and arguably more intuitive name for
`~ultraplot.axes.CartesianAxes.twiny`, which generates
two x axes with a shared ("twin") y axes.

Parameters
----------
**kwargs
    Passed to `~ultraplot.axes.CartesianAxes`. Supports all valid
    `~ultraplot.axes.CartesianAxes.format` keywords. You can optionally
    omit the x from keywords beginning with ``x`` -- for example
    ``ax.altx(lim=(0, 10))`` is equivalent to ``ax.altx(xlim=(0, 10))``.
    You can also change the default side for the axis spine, axis tick marks,
    axis tick labels, and/or axis labels by passing ``loc`` keywords. For example,
    ``ax.altx(loc='bottom')`` changes the default side from top to bottom.

Returns
-------
ultraplot.axes.CartesianAxes
    The resulting axes.

Note
----
This enforces the following default settings:

* Places the old x axis on the bottom and the new x
  axis on the top.
* Makes the old top spine invisible and the new bottom, left,
  and right spines invisible.
* Adjusts the x axis tick, tick label, and axis label positions
  according to the visible spine positions.
* Syncs the old and new y axis limits and scales, and makes the
  new y axis labels invisible.
File:      ~/mambaforge/envs/jax-galsim/lib/python3.10/site-packages/ultraplot/gridspec.py
Type:      method

Do we really want to change this? The new doc strings you wrote a very much not helpful.

cvanelteren · 2025-06-24T11:32:55Z

I see, ok let's go with that.

cvanelteren · 2025-06-24T21:02:36Z

Should be good now:

altx(*args, **kwargs) -> 'SubplotGrid' method of ultraplot.gridspec.S
ubplotGrid instance
    Add an axes locked to the same location with a
    distinct x axis for every axes in the grid.
    This is an alias and arguably more intuitive name for
    `~ultraplot.axes.CartesianAxes.twiny`, which generates
    two x axes with a shared ("twin") y axes.

[... etc]

cvanelteren added 4 commits June 21, 2025 12:25

mv dynamic function to the subplotgrid

7d61a4c

replace with stub

a69cb29

rm dead code and move decorator up

8b8783a

add missing functions and re-add dep warning

97793f9

beckermr previously requested changes Jun 21, 2025

View reviewed changes

cvanelteren added 2 commits June 21, 2025 13:10

rm dead code

0e46e5f

init some unittests

c1e8965

cvanelteren requested a review from beckermr June 21, 2025 11:42

add type hinting

4b67cd6

cvanelteren added 2 commits June 22, 2025 10:48

rm dead code and make implementation more explicit

c26e61e

add one more comment

c4abd27

cvanelteren marked this pull request as draft June 22, 2025 09:07

add more tests

392a324

cvanelteren marked this pull request as ready for review June 22, 2025 10:11

also check panel

47ffa69

cvanelteren mentioned this pull request Jun 22, 2025

Tests failing consistency #274

Closed

rename grid command to apply_to_all

b61e8e2

beckermr requested changes Jun 23, 2025

View reviewed changes

Update ultraplot/gridspec.py

28b26fc

Co-authored-by: Matthew R. Becker <[email protected]>

beckermr requested changes Jun 24, 2025

View reviewed changes

update wrapper to dynamically update the doc string

82789a5

cvanelteren added 4 commits June 24, 2025 23:05

add comment

8ec21d2

update spelling

4a93bd1

update test

55db892

Merge branch 'main' into refactor-gridspec

bf57165

beckermr merged commit c9507ff into Ultraplot:main Jun 25, 2025
25 checks passed

cvanelteren added a commit that referenced this pull request Aug 14, 2025

Mv dynamic function to the subplotgrid (#281)

25ee524

cvanelteren added a commit that referenced this pull request Jan 16, 2026

Mv dynamic function to the subplotgrid (#281)

2a33368

Mv dynamic function to the subplotgrid #281

Mv dynamic function to the subplotgrid #281

Uh oh!

Conversation

cvanelteren commented Jun 21, 2025

Uh oh!

beckermr left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cvanelteren commented Jun 21, 2025

Uh oh!

cvanelteren commented Jun 21, 2025

Uh oh!

cvanelteren commented Jun 21, 2025

Uh oh!

cvanelteren commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvanelteren commented Jun 23, 2025

Uh oh!

beckermr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beckermr Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

cvanelteren Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

beckermr Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

cvanelteren Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

beckermr Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

cvanelteren Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

cvanelteren Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

beckermr Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beckermr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvanelteren commented Jun 24, 2025

Uh oh!

cvanelteren commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jun 21, 2025 •

edited

Loading

cvanelteren commented Jun 22, 2025 •

edited

Loading

beckermr Jun 24, 2025 •

edited

Loading

beckermr left a comment •

edited

Loading

cvanelteren commented Jun 24, 2025 •

edited

Loading