Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add blank table handling for new returns from heasarc #2624

Merged
merged 3 commits into from Dec 12, 2022
Merged

Add blank table handling for new returns from heasarc #2624

merged 3 commits into from Dec 12, 2022

Conversation

keflavich
Copy link
Contributor

HEASARC is returning FITS files with blank data and a little header metadata as null results now, so we need to handle them.

WIP: requires a test.

@keflavich keflavich marked this pull request as draft December 9, 2022 23:16
@keflavich keflavich marked this pull request as ready for review December 9, 2022 23:27
@keflavich keflavich mentioned this pull request Dec 9, 2022
Closed
@bsipocz
Copy link
Member

bsipocz commented Dec 11, 2022

@keflavich - can you rebase, that should fix CI.

@bsipocz bsipocz added this to the v0.4.7 milestone Dec 11, 2022
@bsipocz bsipocz linked an issue Dec 11, 2022 that may be closed by this pull request
BUG: HEASARC query_region to parse image responses #2560
Closed
@keflavich
Copy link
Contributor Author

Now I'm confused. The test code is explicitly catching the warning that is getting raised, following the python guidelines, but the test code is still failing on the warning.

bsipocz
bsipocz requested changes Dec 12, 2022
View reviewed changes
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

This will need a changelog entry. And could you squash the testing iterations a bit?

And I see some remote tests failings

astroquery/heasarc/tests/test_heasarc_remote.py Outdated
"""
heasarc = Heasarc()

with warnings.catch_warnings(record=True) as warn:
Copy link
Member

Choose a reason for hiding this comment

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

why not use pytest.warns here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I didn't remember it was a thing

@keflavich
Copy link
Contributor Author

once this passes, I'll squash

@codecov
Copy link

codecov bot commented Dec 12, 2022 •
edited

Codecov Report

Merging #2624 (eda8446) into main (a867890) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head eda8446 differs from pull request most recent head 0b89749. Consider uploading reports for the commit 0b89749 to get more accurate results

@@            Coverage Diff             @@
##             main    #2624      +/-   ##
==========================================
+ Coverage   64.33%   64.35%   +0.02%     
==========================================
  Files         130      130              
  Lines       16836    16846      +10     
==========================================
+ Hits        10832    10842      +10     
  Misses       6004     6004
Impacted Files Coverage Δ
astroquery/heasarc/core.py 75.48% <100.00%> (+1.69%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@keflavich
add a remote regression test
8843eb9 
remove unused variable

add remote test and restructure fallback

catch warning

import warnings

try changing the warning filter to 'always'

check in message, not in str

rearrange warning test

DEBUG

reading the instructions tells you how to use the tool.

fix to use pytest.warns
@keflavich
changelog entry
0b89749 
fix obvious typo

remove unused import

apparently I had comment row 0 wrong?

add some comments in case the remote test fails for good reason in the future [ci skip]
@keflavich
Copy link
Contributor Author

it passed, I squashed, has changelog. Going to bed. $5 says it'll break and I'll need to do more in the morning.

@bsipocz
Copy link
Member

bsipocz commented Dec 12, 2022

it passed, I squashed, has changelog. Going to bed. $5 says it'll break and I'll need to do more in the morning.

haha, the only thing that breaks atm is that in the squash you left the [skip ci] in the commit message, so tests are not running. Otherwise I would think this passes, so I do a review and force the CI to pass to get this in before you wake up 😉

bsipocz
bsipocz approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

This is all green now, and changes look good. Ideally the problem should be reported upstream (no results wrapped in a fitsfile is more than weird), but that's out of scope for the PR.

@bsipocz bsipocz merged commit 7de626b into astropy:main Dec 12, 2022
@bsipocz
Copy link
Member

bsipocz commented Dec 12, 2022

Thanks Adam!

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

@bsipocz bsipocz bsipocz approved these changes

Assignees
No one assigned
Labels
bug heasarc
Projects
None yet
Milestone
v0.4.7
Development

Successfully merging this pull request may close these issues.

BUG: HEASARC query_region to parse image responses
2 participants
@keflavich @bsipocz