Skip to content

Raise ValueError instead of a deprecation warning for unrecognized spine positions - #20782

Draft
astrofrog wants to merge 2 commits into
astropy:mainfrom
astrofrog:wcsaxes-fix-11929
Draft

astrofrog wants to merge 2 commits into
astropy:mainfrom
astrofrog:wcsaxes-fix-11929

Conversation

@astrofrog

@astrofrog astrofrog commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

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

  • I certify that I am human and that I take full responsibility for this pull request including all interactions with reviewers.

Merge method

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@astrofrog astrofrog added this to the v9.0.0 milestone Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@astrofrog astrofrog added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Nov 5, 2026
@astrofrog
astrofrog requested a review from Cadair November 5, 2026 12:03
@pllim
pllim marked this pull request as draft November 5, 2026 17:00

@larrybradley larrybradley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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, and set_axislabel_position docstrings still say positions should be b/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_params applies the size/width/color settings before calling set_ticks_position, so e.g. ax.coords[0].tick_params(length=10, left=True) on a RectangularFrame1D now 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(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 "

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period visualization.wcsaxes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wcsaxes.coords.set_axeslabel_position not working for y axis

3 participants