Skip to content

Remove Non-functional Overlay Args from Skyview#2979

Merged
bsipocz merged 3 commits into
astropy:mainfrom
duytnguyendtn:drop_overlay_args
May 9, 2024
Merged

Remove Non-functional Overlay Args from Skyview#2979
bsipocz merged 3 commits into
astropy:mainfrom
duytnguyendtn:drop_overlay_args

Conversation

@duytnguyendtn

@duytnguyendtn duytnguyendtn commented Apr 2, 2024

Copy link
Copy Markdown
Contributor

As discussed in #2976, lut, grid, and gridlabel arguments only apply for JPEG output from Skyview, though Astroquery only returns FITS objects. These values are effectively non-functional for Astroquery. This PR removes those arguments as arguments from the relevant Skyview methods and updates the docstring accordingly. Skyview tests and codestyle checks were checked and passing.

Changelog will be updated once this PR goes live and a PR number is generated.

Thanks!
Duy Nguyen (NASA HEASARC)

Fixes #2976

@codecov

codecov Bot commented Apr 2, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.81%. Comparing base (886817e) to head (81cdbad).
Report is 94 commits behind head on main.

❗ Current head 81cdbad differs from pull request most recent head b3f3139. Consider uploading reports for the commit b3f3139 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2979      +/-   ##
==========================================
- Coverage   66.83%   66.81%   -0.02%     
==========================================
  Files         237      237              
  Lines       18327    18323       -4     
==========================================
- Hits        12248    12243       -5     
- Misses       6079     6080       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@duytnguyendtn

Copy link
Copy Markdown
Contributor Author

I believe the failing doctests are unrelated

@keflavich keflavich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As noted in the referenced issue: I'm in favor of removing these w/o deprecation because they never did anything.

@bsipocz bsipocz added the skyview label Apr 6, 2024
@bsipocz bsipocz added this to the v0.4.8 milestone Apr 6, 2024
@duytnguyendtn

Copy link
Copy Markdown
Contributor Author

Rebased at the request of @bsipocz! I believe the test failures are the same as in main: https://github.com/astropy/astroquery/actions/runs/8964265630/job/24664194149#step:5:640

@bsipocz bsipocz 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.

OK, let's have this in as is without a deprecation.

If we get any user reports during the 0.4.8 dev cycle about the exception they now receive we can roll back and do it properly with a deprecation and a warning.

Comment thread CHANGES.rst Outdated
@bsipocz

bsipocz commented May 9, 2024

Copy link
Copy Markdown
Member

CI failure is unrelated and should be addressed separately.

Thank you @duytnguyendtn, and welcome on board!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SkyView Overlay Options not working

3 participants