Fix issue in ndcube requiring changes in astropy by ViciousEagle03 · Pull Request #15985 · astropy/astropy

ViciousEagle03 · 2024-02-04T05:20:22Z

Please use SQUASH AND MERGE button if we decide to merge. Thanks!

Description

This pull request address issue sunpy/ndcube#426 of the ndcube repo in Sunpy Organisation,
Issue link: sunpy/ndcube#426

Describe issue:
When an instance of Coordinatehelper class is created passing draw_grid=False to the associate coordinate helpers from the axes instance emitted by plot() is ignored.

The solution:
Requires re-structuring of code in coordinate_helpers.py so that all the cases for draw_grid is met

github-actions · 2024-02-04T05:20:53Z

github-actions

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].

neutrinoceros

Your patch seems sound on the first look, however it doesn't look backward compatible yet as it's breaking some image comparison tests.
I was able to track down the reason why this happens: the WCSAxes.grid method sets show_grid=None by default (which is equivalent to False in contexts where it's interpreted as a boolean), whic gets passed down to CoordinateHelper.grid.

astropy/astropy/visualization/wcsaxes/core.py

Line 771 in 3d5aae9

def grid(self, b=None, axis="both", *, which="major", **kwargs):

I think you'll need to explicitly distinguish draw_grid=False and draw_grid=None.

Your patch also makes it more evident that the visible keyword argument is silently dropped (which is already the case in the current version). It appears this quirk emerged as matplotlib's API evolved (the same parameter used to be called b and renamed to visible) but derived classes in astropy didn't follow. That's a problem on in own so I'll probably just open a separate issue and we don't need to fix it here, but I want to thank you for making me realise this !

…separately and separately

ViciousEagle03 · 2024-02-04T14:50:18Z

I have handled the case of draw_grid = False and draw_grid = None separately. Specifically when draw_grid = None , I have refrained from changing the value of visible.

neutrinoceros · 2024-02-04T15:03:39Z

Thank you ! it's still failing though, so there's more to it than I thought, sorry !

pllim · 2024-02-05T21:08:13Z

The failures look related. Either the "truths" been wrong all this time or this patch is incorrect. cc @astrofrog @Cadair @dstansby

Example log: https://output.circle-artifacts.com/output/job/73e989ee-f7aa-46a0-9c00-484a6ab06246/artifacts/0/results/fig_comparison.html

neutrinoceros

Thanks for fixing tests !
I think we should also add a new one to prevent regressions, and a changelog entry is also needed to merge this !

ViciousEagle03 · 2024-02-07T12:50:33Z

I have added the changelog, reorganized the patch, and included the regression test as requested.(I think I'll need more time to properly write the test )

pllim · 2024-02-07T14:43:53Z

I have added the changelog, reorganized the patch, and included the regression test as requested.

Perhaps you forgot to push those additions out? I do not see any change log or test.

Also please refrain from pushing your commits out one by one. I would encourage you to test your fix locally first. Thanks!

ViciousEagle03 · 2024-02-07T15:36:14Z

Yes, will do
I thought dividing the commits into smaller parts was a good practice, but I'll make sure to test my fix locally first before pushing them out together.

pllim · 2024-02-07T16:42:30Z

For a change that is only +5 -4, one commit is usually enough.

ViciousEagle03 · 2024-02-07T20:04:02Z

I understand , appreciate your guidance!

Co-authored-by: Clément Robert <[email protected]>

ViciousEagle03 · 2024-02-10T10:32:13Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

ViciousEagle03 · 2024-02-10T14:48:42Z

@neutrinoceros, would you mind taking a look at the recent changes? Thank you!

neutrinoceros

Apart from a small formatting issue with the changelog fragment, this now looks good to me. In particular, I've checked that both tests fail as expected on the main branch. Thank you for your work and patience !

Co-authored-by: Clément Robert <[email protected]>

ViciousEagle03 · 2024-02-10T17:33:38Z

Thank you for guiding me through the process.
Apologies for multiple commits.

neutrinoceros · 2024-02-10T20:17:48Z

No problem !

neutrinoceros

let me re-approve this

Co-authored-by: P. L. Lim <[email protected]>

pllim

Much simpler now, so LGTM. Thanks!

Would be nice if @Cadair or @dstansby can double check since sunpy was mentioned in the original issue that this PR is supposed to fix.

* Fix issue in ndcube requiring changes in astropy * Fix issue in ndcube (required changes in astropy)Handled the case of separately and separately * Fix issue in ndcube (required changes in astropy) * Fix issue in ndcube (required changes in astropy) * Restructured the patch * Add changlog and regression test * Update docs/changes/visualization/15985.bugfix.rst Co-authored-by: Clément Robert <[email protected]> * Corrected edge case * Modified tests for my patch * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update docs/changes/visualization/15985.bugfix.rst Co-authored-by: Clément Robert <[email protected]> * Removed the case where draw_grid value is invalid * Update docs/changes/visualization/15985.bugfix.rst Co-authored-by: P. L. Lim <[email protected]> --------- Co-authored-by: Clément Robert <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: P. L. Lim <[email protected]> Co-authored-by: Stuart Mumford <[email protected]>

Fix issue in ndcube requiring changes in astropy

4983c06

ViciousEagle03 requested review from Cadair, astrofrog and larrybradley as code owners February 4, 2024 05:20

github-actions Bot added the visualization.wcsaxes label Feb 4, 2024

github-actions Bot reviewed Feb 4, 2024

View reviewed changes

neutrinoceros requested changes Feb 4, 2024

View reviewed changes

Fix issue in ndcube (required changes in astropy)Handled the case of …

11ebefa

…separately and separately

neutrinoceros mentioned this pull request Feb 4, 2024

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

Closed

pllim added this to the v6.1.0 milestone Feb 5, 2024

pllim added the Bug label Feb 5, 2024

ViciousEagle03 added 2 commits February 7, 2024 11:24

Fix issue in ndcube (required changes in astropy)

00bd9e8

Fix issue in ndcube (required changes in astropy)

6ab5d09

neutrinoceros reviewed Feb 7, 2024

View reviewed changes

Comment thread astropy/visualization/wcsaxes/coordinate_helpers.py Resolved

neutrinoceros reviewed Feb 7, 2024

View reviewed changes

Restructured the patch

06151f6

ViciousEagle03 force-pushed the my-own-astropy branch 2 times, most recently from a859cae to 06151f6 Compare February 7, 2024 13:55

Add changlog and regression test

939531f

neutrinoceros reviewed Feb 7, 2024

View reviewed changes

Comment thread docs/changes/visualization/15985.bugfix.rst Resolved

Update docs/changes/visualization/15985.bugfix.rst

8218fac

Co-authored-by: Clément Robert <[email protected]>

neutrinoceros reviewed Feb 8, 2024

View reviewed changes

Comment thread astropy/visualization/wcsaxes/coordinate_helpers.py Resolved

ViciousEagle03 added 3 commits February 8, 2024 22:03

Corrected edge case

99f831c

Merge remote-tracking branch 'origin/my-own-astropy' into my-own-astropy

e3a4ce6

Modified tests for my patch

6101848

[pre-commit.ci] auto fixes from pre-commit.com hooks

113f6e0

for more information, see https://pre-commit.ci

neutrinoceros approved these changes Feb 10, 2024

View reviewed changes

Comment thread docs/changes/visualization/15985.bugfix.rst Resolved

Update docs/changes/visualization/15985.bugfix.rst

ed8374f

Co-authored-by: Clément Robert <[email protected]>

pllim reviewed Feb 12, 2024

View reviewed changes

Comment thread astropy/visualization/wcsaxes/coordinate_helpers.py Resolved

Removed the case where draw_grid value is invalid

e0d9e0a

neutrinoceros approved these changes Feb 14, 2024

View reviewed changes

pllim reviewed Feb 14, 2024

View reviewed changes

Comment thread docs/changes/visualization/15985.bugfix.rst Resolved

Update docs/changes/visualization/15985.bugfix.rst

f20eb19

Co-authored-by: P. L. Lim <[email protected]>

pllim approved these changes Feb 14, 2024

View reviewed changes

Cadair approved these changes Mar 19, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into my-own-astropy

b957cfd

Cadair enabled auto-merge (squash) March 19, 2024 09:58

Cadair merged commit 476358b into astropy:main Mar 19, 2024

Conversation

ViciousEagle03 commented Feb 4, 2024 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Feb 4, 2024

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

ViciousEagle03 commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Feb 4, 2024

Uh oh!

pllim commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

ViciousEagle03 commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Feb 7, 2024

Uh oh!

ViciousEagle03 commented Feb 7, 2024

Uh oh!

pllim commented Feb 7, 2024

Uh oh!

ViciousEagle03 commented Feb 7, 2024

Uh oh!

Uh oh!

Uh oh!

ViciousEagle03 commented Feb 10, 2024

Uh oh!

ViciousEagle03 commented Feb 10, 2024

Uh oh!

neutrinoceros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViciousEagle03 commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Feb 10, 2024

Uh oh!

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ViciousEagle03 commented Feb 4, 2024 •

edited by pllim

Loading

ViciousEagle03 commented Feb 4, 2024 •

edited

Loading

pllim commented Feb 5, 2024 •

edited

Loading

ViciousEagle03 commented Feb 7, 2024 •

edited

Loading

neutrinoceros left a comment •

edited

Loading

ViciousEagle03 commented Feb 10, 2024 •

edited

Loading