GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Move axisartist towards untransposed transforms (operating on (N, 2) arrays instead of (2, N) arrays). by anntzer · Pull Request #27551 · matplotlib/matplotlib

Conversation

@anntzer

@anntzer anntzer commented Dec 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

While Matplotlib normally represents lists of (x, y) coordinates as (N, 2) arrays and transforms (which we'll call "trans") have shape signature (N, 2) -> (N, 2), axisartist uses the opposite convention of using (2, N) arrays (or size-2 tuples of 1D arrays) and transforms (which it typically calls "transform_xy"). Change that and go back to Matplotlib's standard represenation in some of axisartist's internal representations for consistency. Also replace some uses of (x1, y1, x2, y2) quadruplets by single Bbox objects, which avoid having to keep track of the order of the points (is it x1, y1, x2, y2 or x1, x2, y1, y2?).

  • Add a _find_transformed_bbox(trans, bbox) API to ExtremeFinderSimple and its subclasses, replacing __call__(transform_xy, x1, y1, x2, y2). (I intentionally did not overload __call__'s signature yet nor did I deprecate it for now; we can consider doing that later.)
  • Deprecate GridFinder.{,inv_}transform_xy, which implement the transposed transform API.
  • Switch grid_info["extremes"] from quadruplet representation to Bbox.
  • Switch grid_info["lon"]["lines"] and likewise for "lat" from list-of-size-1-lists-of-pairs-of-1D-arrays to list-of-(N, 2)-arrays.
  • Switch grid_info["line_xy"] from pair-of-1D-arrays to a (N, 2) array.
  • Let _get_raw_grid_lines take a Bbox as last argument instead of 4 coordinates.

Note that I intentionally mostly didn't touch (transpose) public-facing APIs for now, this may happen later.

PR summary

PR checklist

  • "closes #0000" is in the body of the PR description to link the related issue
  • new and changed code is tested
  • Plotting related features are demonstrated in an example
  • New Features and API Changes are noted with a directive and release note
  • Documentation complies with general and docstring guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@anntzer anntzer added the topic: mpl_toolkit label Dec 20, 2023
@anntzer
anntzer force-pushed the aaut branch from 0bcd2af to 29dc43a Compare December 20, 2023 22:47
oscargus
oscargus reviewed Dec 21, 2023
View reviewed changes
Comment thread lib/mpl_toolkits/axisartist/floating_axes.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the aaut branch from 29dc43a to 7c55684 Compare December 21, 2023 09:09
greglucas
greglucas approved these changes Dec 17, 2024
View reviewed changes

@greglucas greglucas left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Overall this is a nice refactor to use our internal transfrom tooling rather than reinventing it all. Just some minor suggestions from me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/mpl_toolkits/axisartist/floating_axes.py Outdated
Comment on lines +200 to +202
lon_min, lat_min = tbbox.min
lon_max, lat_max = tbbox.max
grid_info["extremes"] = Bbox.from_extents(lon_min, lat_min, lon_max, lat_max)

@greglucas greglucas Dec 17, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You use .extents later on which seems like you could use it here too?

Perhaps just store the frozen version which essentially does this for you, or do we even need it be "frozen" or can you put the bbox directly into the grid_info dictionary? (similar in the from_extents() call later on too)

Suggested change
lon_min, lat_min = tbbox.min
lon_max, lat_max = tbbox.max
grid_info["extremes"] = Bbox.from_extents(lon_min, lat_min, lon_max, lat_max)
lon_min, lat_min, lon_max, lat_max = tbbox.extents
grid_info["extremes"] = tbbox.frozen()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@anntzer anntzer Dec 17, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good catch, changed.
I also further swapped the signature of _update_grid to also just take a single Bbox as parameter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/mpl_toolkits/axisartist/grid_finder.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the aaut branch from 7c55684 to 4800e11 Compare December 17, 2024 21:10
@timhoffm

timhoffm commented Dec 18, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

While Matplotlib normally represents lists of (x, y) coordinates as (N, 2) arrays [...], axisartist uses the opposite convention of using (2, N) arrays (or size-2 tuples of 1D arrays)
Note that I intentionally mostly didn't touch (transpose) public-facing APIs for now, this may happen later.

On a side-note, we can (and maybe should?) distinguish/handle (N, 2) arrays and (size-2 tuples of 1D arrays) for the API. That (size-2 tuples of 1D arrays) is technically a 2D array-like does not mean we have to interpret it in the same way as a 2D array.

Of course internally, (N, 2) arrays are the way to go, but for user-facing API not all array-likes have to be interpreted the same way. For example, one currently has to do

x = np.linspace(0, 6.28)
y = np.sin(x)
points = np.array([x, y]).T
Path(points)

The np.array(...).T feels quite awkward and is something that new users have to learn.

(Disregarding backward compatibility for now), I think it's not unreasonable to also allow

points = (x, y)
Path(points)

Note that the distinction would be very specifically limited to tuple of array-like, i.e. an implementation would likely coerce using

if isinstance(points, tuple):
    # assume tuple of coordinate arrays (x, y, ...)
    points = np.array(*points).T
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer

anntzer commented Dec 18, 2024

Copy link
Copy Markdown
Contributor Author

The np.array(...).T feels quite awkward and is something that new users have to learn.

Personally I always write np.column_stack([x, y]) which makes more semantic sense to me.

Note that the distinction would be very specifically limited to tuple of array-like, i.e. an implementation would likely coerce using

There are currently some APIs (e.g., boxplot) that distinguish between lists (or tuples) of lists and 2d ndarrays, and they cause endless confusion... See e.g. #2539, #8092.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timhoffm

timhoffm commented Dec 18, 2024

Copy link
Copy Markdown
Member

Personally I always write np.column_stack([x, y]) which makes more semantic sense to me.

Indeed.

There are currently some APIs (e.g., boxplot) that distinguish between lists (or tuples) of lists and 2d ndarrays, and they cause endless confusion... See e.g. #2539, #8092.

Noted. Though I'm unclear how endless this trouble actually is. #2539 is a single report. #8092 is specifically on array of arrays, which is not really specified in the docs. I'd consider the implementation a bug and would simply disallow array of arrays. Also note that specifically boxplot() must support different number of data points per box and thus a 2D array is not sufficient, OTOH data are often in the form of 2D arrays, so accepting that is reasonable as well (and related but without discussion aggregating along columns).

I don't want to open a full discussion now. I agree with the scope of the PR that we don't want 2D (2, N) arrays.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

timhoffm
timhoffm reviewed Dec 19, 2024
View reviewed changes
Comment thread lib/mpl_toolkits/axisartist/grid_finder.py
dy = (y_max - y_min) / self.ny
return x_min - dx, x_max + dx, y_min - dy, y_max + dy
def _find_transformed_bbox(self, trans, bbox):
grid = np.reshape(np.meshgrid(np.linspace(bbox.x0, bbox.x1, self.nx),

@timhoffm timhoffm Dec 19, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Even though this is private, it should have a docstring.

In particular it should explain the padding (expanded()) and it should mention that it's semantically equivalent to __call__ but with a better API.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@anntzer anntzer Dec 19, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@anntzer
Move axisartist towards untransposed transforms.
a83af6a
While Matplotlib normally represents lists of (x, y) coordinates as
(N, 2) arrays and transforms (which we'll call "trans") have shape
signature (N, 2) -> (N, 2), axisartist uses the opposite convention
of using (2, N) arrays (or size-2 tuples of 1D arrays) and transforms
(which it typically calls "transform_xy").  Change that and go back to
Matplotlib's standard represenation in some of axisartist's internal
representations for consistency.  Also replace some uses of (x1, y1, x2,
y2) quadruplets by single Bbox objects, which avoid having to keep track
of the order of the points (is it x1, y1, x2, y2 or x1, x2, y1, y2?).

- Add a `_find_transformed_bbox(trans, bbox)` API to ExtremeFinderSimple
  and its subclasses, replacing `__call__(transform_xy, x1, y1, x2, y2)`.
  (I intentionally did not overload `__call__`'s signature yet nor did I
  deprecate it for now; we can consider doing that later.)
- Deprecate `GridFinder.{,inv_}transform_xy`, which implement the
  transposed transform API.
- Switch `grid_info["extremes"]` from quadruplet representation to Bbox.
- Switch `grid_info["lon"]["lines"]` and likewise for "lat" from
  list-of-size-1-lists-of-pairs-of-1D-arrays to list-of-(N, 2)-arrays.
- Switch `grid_info["line_xy"]` from pair-of-1D-arrays to a (N, 2) array.
- Let `_get_grid_info` and `_get_raw_grid_lines` take a Bbox as (last)
  argument instead of 4 coordinates.

Note that I intentionally mostly didn't touch (transpose) public-facing
APIs for now, this may happen later.
@anntzer
anntzer force-pushed the aaut branch from 4800e11 to a83af6a Compare December 19, 2024 21:33
timhoffm
timhoffm approved these changes Dec 19, 2024
View reviewed changes
@timhoffm timhoffm added this to the v3.11.0 milestone Dec 19, 2024
@timhoffm
timhoffm merged commit 82adc45 into matplotlib:main Dec 19, 2024
@anntzer
anntzer deleted the aaut branch December 20, 2024 09:23
@aduan5567-netizen aduan5567-netizen mentioned this pull request Sep 20, 2026
Fix s-grid rendering on Matplotlib 3.11 by disabling longitude unwrapping python-control/python-control#1250
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oscargus oscargus oscargus left review comments
@timhoffm timhoffm timhoffm approved these changes
@greglucas greglucas greglucas approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic: mpl_toolkit

Projects

None yet

Milestone

v3.11.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@anntzer @timhoffm @oscargus @greglucas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.