astropy / astropy/astropy

BUG: WCSAxes.grid's signature is incompatible with parent class

Open
#15,987 2 comments 0 reactions 0 assignees View on GitHub
API change Bug visualization.wcsaxes
Dominant language
Python
Stars
5.3k
Forks
2.2k
Avg merge
1d 19h
Merged PRs (30d)
75

Description

### Description

`astropy.visualization.wcsaxes.core.WCSAxes` is derived from `matplotlib.axes.Axes`.

With time, `WCSAxes.grid`'s signature drifted apart from its parent class', leading to a LSP (Liskov Substitution Principle) violation.
Here's the current document for the parent method
https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.grid.html#matplotlib.axes.Axes.grid

And here's the child definition
https://github.com/astropy/astropy/blob/3d5aae9d0b435c459d4e10c4958c73568eda6552/astropy/visualization/wcsaxes/core.py#L771

1) the first argument `b` was renamed `visible` (as a boolean, it's common style to pass it as keyword, which makes it a problem)
2) `which` is accepted as positional in the parent method but is keyword-only in the child method
3) `axis` is accepted positionally in both, but is on third position in the parent method and on second position in the child method

The most recent change (`b` renamed to `visible`) happened in matplotlib v3.5.0, and we currently support matplotlib 3.3, so there's an additional layer of complexity if we want to match the parent definition in a version-dependent way.
Luckily, matplotlib 3.6 is currently the oldest version recommended by [SPEC 0](https://scientific-python.org/specs/spec-0000/) so we could just bump the requirement.

The question is how to address these incompatibilities gradually (as opposed to just changing the signature in one go, which would be a breaking change)
Problem 1) can be solved using `@deprecated_renamed_argument`, however it's not the case of problems 2) and 3), which I'm not sure *can* be resolved over a deprecation period. I'm thinking we could just resolve problem 1) in a minor release, and 2) and 3) would need to be addressed in a major one.

A related issue is that `astropy.visualization.wxsaxes.coordinate_helpers.CoordinateHelper.grid` takes a `draw_grid` argument (which is equivalent to the `visible` argument) *and* arbitrary keyword arguments (`**kwargs`) that are passed from `astropy.visualization.wcsaxes.core.WCSAxes.grid`, in a way that passing `visible` as keyword in `astropy.visualization.wcsaxes.core.WCSAxes.grid` has no effect.

Originally discussed in https://github.com/astropy/astropy/pull/15985#pullrequestreview-1861391239

### Expected behavior

_No response_

### How to Reproduce

_No response_

### Versions

_No response_

Contributor guide

Open the contributing guide

Research direction

Compare astropy/visualization/wcsaxes/core.py around WCSAxes.grid with matplotlib.axes.Axes.grid, including the supported matplotlib 3.3 version and the 3.6 recommendation. Then read astropy/visualization/wcsaxes/coordinate_helpers.py and CoordinateHelper.grid to trace how visible, draw_grid, axis, which, and **kwargs are handled. Done means an agreed compatibility and deprecation path that preserves intended grid behavior across supported versions.

Written by the indexing model from the issue text.

Assessment

Tech stack
matplotlib, python
Domain
data-visualization
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.