BUG: WCSAxes.grid's signature is incompatible with parent class
- 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
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