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

Maintenance for the VizieR module #3028

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Conversation

ManonMarchand
Copy link
Member

Hi astroquery,

What the PR does

This is just a maintenance PR. It does:

  • allow the tests to run in the documentation
  • switch from -meta.all to -meta in find_catalogs (the extended information is not in the <RESOURCE> tag anyway, so it is not used by the method). This increases the speed of this method and reduces the size of the downloaded information a lot
  • update the remote tests with examples that return less results to make them a bit faster, and reduce the impact on VizieR's servers

Internally, to use the -meta option that has no value (a weird thing of the ASU protocol) I had to change the post request in find_catalog. This has the added benefit that we can now use the -obsolete option rather than filtering afterwards.

There is no change to the API.

On the ucd test failure

This is also there on main. It is an upstream issue with the votable output (discovered thanks to this PR), most likely introduced in VizieR v7.33.3 . It is already solved in the beta version of VizieR, so the fix will be here automatically with the next release upstream.

@ManonMarchand
Copy link
Member Author

Strange. Is there no action running the remote tests? I know that the UCD test fails.

@bsipocz
Copy link
Member

bsipocz commented Jun 11, 2024

Strange. Is there no action running the remote tests? I know that the UCD test fails.

Nope, we run those once a week (and the job is always failing). In practice I run the relevant remotes before merging a PR to spot any new failures in the affected modules.

@bsipocz bsipocz added this to the v0.4.8 milestone Jun 11, 2024
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.

Thank you so much @ManonMarchand, I really love the cleanups and that the remote tests got some love and care.

None of my comments are blockers, but I leave the PR open for a few more days in case you want to reflect on any of them here.

astroquery/vizier/core.py Outdated Show resolved Hide resolved
astroquery/vizier/tests/test_vizier_remote.py Outdated Show resolved Hide resolved
astroquery/vizier/tests/test_vizier_remote.py Outdated Show resolved Hide resolved
docs/vizier/vizier.rst Outdated Show resolved Hide resolved
@ManonMarchand
Copy link
Member Author

Sure, I'll do these modifications next week, thanks for the review 🙂
(I did not try without ELLIPSIS, that's my fault, there is no bug)

@ManonMarchand ManonMarchand marked this pull request as draft June 13, 2024 12:36
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.17%. Comparing base (462c2da) to head (0ffd5d4).
Report is 66 commits behind head on main.

Current head 0ffd5d4 differs from pull request most recent head 9e85f4e

Please upload reports for the commit 9e85f4e to get more accurate results.

Files Patch % Lines
astroquery/vizier/core.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3028      +/-   ##
==========================================
- Coverage   67.35%   67.17%   -0.18%     
==========================================
  Files         236      231       -5     
  Lines       18320    18247      -73     
==========================================
- Hits        12339    12258      -81     
- Misses       5981     5989       +8     

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

switched to a more specific example than kang 51 because there are often new catalogs matching this
we don't lose information, but ASU-style queries cannot get a dictionnary in the POST request's data
@ManonMarchand ManonMarchand marked this pull request as ready for review June 20, 2024 10:04
@ManonMarchand
Copy link
Member Author

Should be all good?
I did not switch to public API in the non-remote tests since some utility methods are also tested and I don't have the time right now to look at it, but it's on my radar for a future PR

@bsipocz
Copy link
Member

bsipocz commented Jun 20, 2024

I did not switch to public API in the non-remote tests

That's more like a future looking wishlist item at least for the public API stuff as you said for some functionality it doesn't even make sense.

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.

I see 3 remote test failures, but those are also present on main, so will go ahead and merge this now.

Also, there is a doctest failure which I don't see why failing, it maybe a doctestplus bug. Will report it upstream for now.

@bsipocz bsipocz merged commit f64b997 into astropy:main Jun 20, 2024
8 checks passed
@bsipocz
Copy link
Member

bsipocz commented Jun 20, 2024

Thanks @ManonMarchand!

@ManonMarchand ManonMarchand deleted the maint-vizier-module branch June 21, 2024 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants