Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transpose grid_finder tick representation. #27373

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 27, 2023

Instead of representing the information about tick and gridlines as

{
    "lon_lines": [line, ...],  # unused
    "lat_lines": [line, ...],  # unused
    "lon": [
        "tick_levels": [level, ...],
        "tick_locs": {"left": [(xy, angle), ...], "bottom": [(xy, angle), ...], ...},
        "tick_labels": {"left": [label, ...], "bottom": [label, ...], ...},
        "lines": [line, ...],
    ],
    "lat": ...,  # as for lon
}

where the locs and labels are implicitly associated by the iteration order, group the information for each tick, and remove the redundant gridlines info as well:

{
    "lon": {
        "lines": [line, ...],
        "ticks": {
            "left": [
                {"level": level, "loc": (xy, angle), "label": label}, ...
            ],
            "bottom": [...], ...,
        }
    }
    "lat": ...,  # as for lon
}

(This representation turns out to make the implementation quite shorter, and makes more sense to me...)

PR summary

PR checklist

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

It looks good to me, though I do have an implementation question.

for side, crossings in zip(
["left", "right", "bottom", "top"], all_crossings):
for crossing in crossings:
gi["ticks"][side].append({"level": level, "loc": crossing})
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would make sense for this interior dictionary to be a NumPy record array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually that makes sense, but I'm not overly convinced by the patch, which is just

diff --git i/lib/mpl_toolkits/axisartist/grid_finder.py w/lib/mpl_toolkits/axisartist/grid_finder.py
index 897a4152b0..a01a4df53e 100644
--- i/lib/mpl_toolkits/axisartist/grid_finder.py
+++ w/lib/mpl_toolkits/axisartist/grid_finder.py
@@ -208,12 +208,12 @@ class GridFinder:
                 for side, crossings in zip(
                         ["left", "right", "bottom", "top"], all_crossings):
                     for crossing in crossings:
-                        gi["ticks"][side].append({"level": level, "loc": crossing})
+                        gi["ticks"][side].append((level, *crossing, None))
             for side in gi["ticks"]:
-                levs = [tick["level"] for tick in gi["ticks"][side]]
-                labels = self._format_ticks(idx, side, factor, levs)
-                for tick, label in zip(gi["ticks"][side], labels):
-                    tick["label"] = label
+                gi["ticks"][side] = ticks = np.array(
+                    gi["ticks"][side],
+                    [("level", float), ("loc", float, 2), ("angle", float), ("label", object)])
+                ticks["label"] = self._format_ticks(idx, side, factor, ticks["level"])

         return grid_info

diff --git i/lib/mpl_toolkits/axisartist/grid_helper_curvelinear.py w/lib/mpl_toolkits/axisartist/grid_helper_curvelinear.py
index 451e1159ed..d7ed6e8062 100644
--- i/lib/mpl_toolkits/axisartist/grid_helper_curvelinear.py
+++ w/lib/mpl_toolkits/axisartist/grid_helper_curvelinear.py
@@ -83,7 +83,7 @@ class FixedAxisArtistHelper(_FixedAxisArtistHelperBase):
                     (self.nth_coord_ticks, True), (1 - self.nth_coord_ticks, False)]:
                 gi = self.grid_helper._grid_info[["lon", "lat"][nth_coord]]
                 for tick in gi["ticks"][side]:
-                    yield (*tick["loc"], angle_tangent,
+                    yield (*tick[["loc", "angle"]], angle_tangent,
                            (tick["label"] if show_labels else ""))

         return iter_major(), iter([])
@@ -322,8 +322,7 @@ class GridHelperCurveLinear(GridHelperBase):
         lon_or_lat = ["lon", "lat"][nth_coord]
         if not minor:  # major ticks
             for tick in self._grid_info[lon_or_lat]["ticks"][axis_side]:
-                yield *tick["loc"], angle_tangent, tick["label"]
+                yield *tick[["loc", "angle"]], angle_tangent, tick["label"]
         else:
             for tick in self._grid_info[lon_or_lat]["ticks"][axis_side]:
-                yield *tick["loc"], angle_tangent, ""
+                yield *tick[["loc", "angle"]], angle_tangent, ""

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better if we actually used the arrays as a whole, but since the users (in grid_helper_curvelinear.py) are just doing a yield on each row, there doesn't seem to be too much benefit here.

Instead of representing the information about tick and gridlines as
```
{
    "lon_lines": [line, ...],  # unused
    "lat_lines": [line, ...],
    "lon": [
        "tick_levels": [level, ...],
        "tick_locs": {"left": [(xy, angle), ...], "bottom": [(xy, angle), ...], ...},
        "tick_labels": {"left": [label, ...], "bottom": [label, ...], ...},
        "lines": [line, ...],
    ],
    "lat": ...,  # as for lon
}
```
where the locs and labels are implicitly associated by the iteration
order, group the information for each tick, and remove the redundant
gridlines info as well:
```
{
    "lon": {
        "lines": [line, ...],
        "ticks": {
            "left": [
                {"level": level, "loc": (xy, angle), "label": label}, ...
            ],
            "bottom": [...], ...,
        }
    }
    "lat": ...,  # as for lon
}
```
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

Does this need an API change note or not quite public APi?

@anntzer
Copy link
Contributor Author

anntzer commented Dec 6, 2023

It's not touching any public API.

@story645
Copy link
Member

story645 commented Dec 7, 2023

Don't know what to milestone this as so you're welcome to milestone then self merge.

@anntzer
Copy link
Contributor Author

anntzer commented Dec 7, 2023

I'll let @QuLogic chime in as to whether we want to use structured arrays here. In general I like them very much (as much as I don't like pandas 🤪) but here I'm not overly convinced by their use.

@tacaswell tacaswell added this to the v3.9.0 milestone Dec 7, 2023
@QuLogic QuLogic merged commit 8f296db into matplotlib:main Dec 20, 2023
40 checks passed
@anntzer anntzer deleted the gfi branch December 20, 2023 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants