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.

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

Many Vizier fixes #2012

Merged
merged 11 commits into from Mar 20, 2021
Merged

Many Vizier fixes #2012

merged 11 commits into from Mar 20, 2021

Conversation

jwoillez
Copy link
Member

@jwoillez jwoillez commented Mar 10, 2021

This a re-implementation of Vizier.query_region() to support list of coordinates. The API on the Vizier side seems to have changed. This should fix #1962, fix #1826, fix #1502.

@jwoillez
Copy link
Member Author

WIP: need to fix tests.

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #2012 (570ae48) into master (1a89486) will increase coverage by 0.02%.
The diff coverage is 81.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2012      +/-   ##
==========================================
+ Coverage   67.00%   67.02%   +0.02%     
==========================================
  Files         411      411              
  Lines       27149    27164      +15     
==========================================
+ Hits        18192    18208      +16     
+ Misses       8957     8956       -1     
Impacted Files Coverage Δ
astroquery/vizier/core.py 74.72% <78.12%> (+1.07%) ⬆️
astroquery/vizier/tests/test_vizier.py 96.75% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a89486...570ae48. Read the comment docs.

@bsipocz bsipocz added the vizier label Mar 12, 2021
@jwoillez
Copy link
Member Author

jwoillez commented Mar 19, 2021

I should probably have said so earlier: this PR is ready for review.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

Can we keep the test around and fix it so that it works with the current implementation? I prefer not to remove a test when we still support that functionality

@jwoillez
Copy link
Member Author

@keflavich I will try to make it work. I also need to take care of a bug reported in #1962.

@jwoillez
Copy link
Member Author

@keflavich The test I had removed is back. I had to adjust post_mockreturn to handle the new VizieR list format. I also have a fix for the bug reported in #1962.

@jwoillez jwoillez changed the title Fix list handling in Vizier.query_region() Many Vizier fixes Mar 20, 2021
Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

Ready to merge?

@jwoillez
Copy link
Member Author

@keflavich, I have two failing remote tests. With the many changes I have in that PR, I am not too surprised.

@keflavich
Copy link
Contributor

Ah, ok. Are you still working on it, then?

@jwoillez
Copy link
Member Author

I guess so: `Vizier.find_catalog()' is not happy. Let me send a note when all the tests pass.

@jwoillez
Copy link
Member Author

@keflavich Are the remote tests supposed to pass?

@keflavich
Copy link
Contributor

keflavich commented Mar 20, 2021 via email

@jwoillez
Copy link
Member Author

When I run the code of test_findcatalog_maxcatalog() by hand, it works. When I run it through astroquery.test(package='vizier', remote_data='any') it fails. Not sure why...

@jwoillez
Copy link
Member Author

It looks like I haven't changed anything related to Vizier.find_catalogs(). I would say these test might have been failing before... If so, I would say good to review or merge from my side.

@keflavich
Copy link
Contributor

OK. Could you open a new issue noting the specific remote failures, so we don't lose track of them? I'll merge this. Thanks.

@keflavich keflavich merged commit fd93320 into astropy:master Mar 20, 2021
@jwoillez jwoillez deleted the fix_vizier_list branch March 20, 2021 20:24
@bsipocz bsipocz added this to the v0.4.2 milestone May 4, 2021
@bsipocz
Copy link
Member

bsipocz commented May 4, 2021

This would need a changelog (rule of thumb: if there is a user facing issue/API bug report, then we need the entry)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants