Partial rewrite of the MOCServer module

[ManonMarchand]

Hello astroquery,

This is a quite big rewrite of the MOS Server module.

The two main motivations behind it were:

• there was a bug that using query_region with a MOC would write a real file on the people's current folder. This file would be named moc.fits and could potentially overwrite a pre-existing file with the same name silently.
• the MOC server upstream now works with time MOCs and space-time MOCs, so these features were needed here

The actual changes in this PR

• new method query_hips allows to filter only hips in a shortcut rather than having to remember all the time to have to type hips_frame=* in the meta_data (convenience method)
• before, you could only search by region or by meta_data. This was unnecessary, and now the two methods are exactly the same and allow to filter on region and to add a criteria. I was wondering whether find_datasets should be deprecated
• new parameters casesensitive and coordinate_system allow to chose whether the query will respect the case, and to chose a specific system for the datasets (ex: mars, venus, sky)
• new methods list_coordinate_systems that prints the currently available systems (can evolve as providers post new datasets)
• new method list_fields to see the possible fields (there are 134 today) sorted by occurrence and with an example
• the default returned table now have a sub-selection of fields rather than the whole 134 ones (this speeds up the module considerably)
• return_moc is no longer only a boolean. It keeps it former behavior (True means Space MOC) but it can now also take the values smoc, tmoc, or stmoc to specify the kind of MOC that should be returned
• in query_region, the region argument can now also be a mocpy.TimeMOC and a mocpy.STMOC on top of the previously accepted classes
• removed the regions module from the mandatory dependencies

Changes while I was there

• doctesting turned on in the documentation (one box to tick in Enable doctesting for all narrative documentations #2935 )
• remove the **kwargs arguments in all the methods

[pep8speaks]

Hello @ManonMarchand! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-22 23:00:36 UTC

[codecov]

Codecov Report

Attention: Patch coverage is 93.45794% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.57%. Comparing base (1a58a5c) to head (0bed33f).
Report is 391 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/mocserver/core.py	93.39%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3139      +/-   ##
==========================================
- Coverage   67.94%   67.57%   -0.37%     
==========================================
  Files         231      229       -2     
  Lines       19051    18593     -458     
==========================================
- Hits        12945    12565     -380     
+ Misses       6106     6028      -78     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ManonMarchand]

Switching to draft as I asked for a review internally in CDS

[tboch]

Thanks for this PR.
My main comment is about space_sys which I don't like much, and about 'C' which really should be 'sky'.

I see no harm in starting to have a deprecation message for find_datasets.
Should we have a new get_coverage or get_moc method which would be cleaner than the return_moc parameter? This can be added in a later PR of course.

[ManonMarchand]

I applied Thomas's suggestions. This is ready to be reviewed 🙂

[bsipocz]

I'll get back to this in the new year. Thanks, Manon and Happy Holidays!

[bsipocz]

Overall this looks good. I have a few very minor comments. The two must be done are the rebase -- I'm sorry for not getting this into 0.4.8 -- and adding the doctest require to the documentation as the doctests fail for the documentation when there is no mocpy present.

[ManonMarchand]

Hello Brigitta,

Thanks for the review!

No worries for the 0.4.8, we'll get the changes in the next release.

I think I addressed all you comments except the one about meta_data that could be renamed either criteria or dataset_filter depending on what you prefer. I'll also patch the docstring to remove Algebraic once we decide on the new name.

[bsipocz]

I think I addressed all you comments except the one about meta_data that could be renamed either criteria or dataset_filter depending on what you prefer. I'll also patch the docstring to remove Algebraic once we decide on the new name.

Yeap, I'm not sure anymore. I like both your suggestions with a very slight preference for criteria. On the other hand, meta_data is not a new kwarg as I first assumed it was, so I'm now unsure if the rename would be worth the effort of deprecation.

[ManonMarchand]

It's new in query_hips and query_region. The only method that already had meta_data is find_datasets, which is deprecated now anyhow. So I switched to criteria in the two current methods, but kept the old name in the old method.

[bsipocz]

Do you prefer this to go out in 0.4.9 later this week, or keep in main after merge for a little longer?

[ManonMarchand]

I would be better if this could fit in 0.4.9. The bug on overwritting files is really critical

[bsipocz]

OK, let's go ahead with this one. It needs a rebase, and a minor edit to the changelog as I don't think the warning renders out as expected.

[bsipocz]

This warning won't work here, I'll edit this out during rebase.

[bsipocz]

actually, I leave this to the before-release changelog cleanup, and go ahead with the PR as is.

Thanks @ManonMarchand!