GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||
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
|
| 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) |
There was a problem 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)
| 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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 The (Disregarding backward compatibility for now), I think it's not unreasonable to also allow Note that the distinction would be very specifically limited to tuple of array-like, i.e. an implementation would likely coerce using |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Personally I always write
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Indeed.
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 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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), |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sure.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.