Repository navigation
Conversation
…ine positions in WCSAxes
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
larrybradley
left a comment
There was a problem hiding this comment.
Overall, this looks good.
One issue I think needs fixing before this goes in: the hard-coded fallback positions in WCSAxes.reset_wcs (core.py, the ["b", "l"] and ["bltr", "bltr"] defaults used when coord_meta has no default_*_position keys) are only valid for RectangularFrame. Those keys are documented as optional in generic_transforms.rst, so a generic transform with a non-rectangular frame now fails at construction:
import astropy.units as u
import matplotlib.pyplot as plt
from astropy.visualization.wcsaxes import WCSAxes
from astropy.visualization.wcsaxes.frame import EllipticalFrame
from astropy.visualization.wcsaxes.tests.test_transform_coord_meta import DistanceToLonLat
fig = plt.figure()
coord_meta = {"type": ("longitude", "latitude"), "wrap": (180, None),
"unit": (u.deg, u.deg), "name": ("lon", "lat")}
ax = WCSAxes(fig, [0.1, 0.1, 0.8, 0.8], transform=DistanceToLonLat(R=6e3),
coord_meta=coord_meta, frame_class=EllipticalFrame)
# ValueError: Unrecognized position(s): ['b'], should be one of c/h/v.On main this emits three deprecation warnings that the user cannot avoid other than by passing all three default_*_position keys. The same happens with RectangularFrame1D (['l'], should be one of b/t) and with custom frames. Maybe the fallbacks could depend on the frame class, e.g., keep b/l/bltr for RectangularFrame subclasses and use frame_class.spine_names (or "#") otherwise, as wcsapi.py already does? A regression test for the non-rectangular case would be good too.
A couple of minor things outside the diff:
- The
set_ticks_position,set_ticklabel_position, andset_axislabel_positiondocstrings still say positions should beb/t/l/r, which isn't true for non-rectangular frames (pre-existing, but this seems like a good time to fix it). CoordinateHelper.tick_paramsapplies the size/width/color settings before callingset_ticks_position, so e.g.ax.coords[0].tick_params(length=10, left=True)on aRectangularFrame1Dnow updates the tick size and then raises. This is a user error so it may be fine, but validating first would avoid a partial update.
| f"be one of {'/'.join(self.keys())}. In future this will " | ||
| f"raise an error. {hint}", | ||
| AstropyDeprecationWarning, | ||
| raise ValueError( |
There was a problem hiding this comment.
Since any invalid entry now raises, valid_positions always ends up equal to list(positions). The loop could just collect the invalid entries and the method could return list(positions).
| if isinstance(positions, str) and positions in self: | ||
| hint = ( | ||
| f"It looks like '{positions}' matches the name of a single " | ||
| f" It looks like '{positions}' matches the name of a single " |
There was a problem hiding this comment.
Nit: the leading space here works, but it's easy to lose in a future edit. Adding the space where the hint is joined to the message might be a bit more robust.
| ValueError, | ||
| match=r"Unrecognized position\(s\): \['i', 'g', 'h'\], should be one of b/r/t/l", | ||
| ): | ||
| ax.coords[1].set_axislabel_position("right") |
There was a problem hiding this comment.
Nice to have the "right" case from #11929 here. Optionally, the single-axis-name hint is only tested via set_ticks_position. Parametrizing over the three setters would be cheap.
Description
This was introduced in 7.2.0 so can be removed in 9.0.0. Should not be merged until after 8.1.0 is out assuming the next version is 9.0.0
Fixes #11929
AI Disclosure
If AI tools were used to develop this pull request, describe the tools including specific model and version, how they were used, and what content is AI generated. Otherwise enter "N/A".
Assisted by Opus 5.5
Merge method