IPAC/IRSA Moving Object Search Tool (MOST) interface

[DinoBektesevic]

The PR adds support for MOST application at IPAC/IRSA. See the interface here.

There are some instructions, but they do not always map to the exact key names and allowed values. There is an API for the service here but because its output is the same as of the application itself the only functional difference is whether the search parameters are stringified into an URL fragment or not.

Therefore I decided to query the application itself using a POST request. The returned response is parsed, and if HTML that HTML is parsed for the "Download *" links at the bottom of the page. These tables are then returned to the user. This makes "Regular" and "Full" output mode pretty much the same output mode, causing some potential confusion, but is the simplest solution.

I added documentation, could be improved. Same with the tests.

I wasn't sure what to log and and or how exactly to handle errors - I see there are custom made errors for different events but I was too lazy to peruse the docs to be honest. Happy to get any input on that.

I also wasn't sure what all parameters should go into the Config class. So now some of them are in the default dict for the class and some are in config - happy to get advice on the recommended way forward.

There is a point to be made for unifying all of the same-meaning-but-different-named-parameters used by MOST (input_type but output_mode, fits_region_files for create_tarballs etc..) but that seemed like not the best idea - happy to hear otherwise.

EDIT: I see now I forgot to add list_catalogs and get_images as methods. Also, what should I name the main query method - I was not sure which example in the API Reference was the closest match?

[codecov]

Codecov Report

Merging #2660 (5f72e7e) into main (aa9ce56) will increase coverage by 0.07%.
The diff coverage is 77.41%.

❗ Current head 5f72e7e differs from pull request most recent head c07e1b8. Consider uploading reports for the commit c07e1b8 to get more accurate results

@@            Coverage Diff             @@
##             main    #2660      +/-   ##
==========================================
+ Coverage   66.04%   66.11%   +0.07%     
==========================================
  Files         233      234       +1     
  Lines       17927    18048     +121     
==========================================
+ Hits        11840    11933      +93     
- Misses       6087     6115      +28     

Impacted Files	Coverage Δ
astroquery/ipac/irsa/most.py	76.27% <76.27%> (ø)
astroquery/ipac/irsa/__init__.py	100.00% <100.00%> (ø)
astroquery/ipac/irsa/core.py	77.61% <100.00%> (ø)

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

[DinoBektesevic]

I can't seem to reproduce the error locally.

Error:

UNEXPECTED EXCEPTION: ConnectionError(MaxRetryError("HTTPSConnectionPool(host='irsa.ipac.caltech.edu', port=443): Max retries exceeded with url: /cgi-bin/MOST/nph-most (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f04900aad90>: Failed to establish a new connection: An attempt was made to connect to the internet by a test that was not marked `remote_data`. The requested host was: 134.4.54.110'))"))

On the lines

353 .. doctest-remote-data::
354 
355     >>> from astroquery.ipac.irsa.most import Most
356     >>> Most.query_object(output_mode="Brief",

in most.core.py

[bsipocz]

Overall this PR is very close to be ready.

Leaving some big picture comments, they are mostly logistics and nitpicks. I'll do a run on the docs once it's been moved around.

[bsipocz]

mandate all optional kwargs to be kwarg only, here and in all the other methods, too (at minimum in the public ones).

[DinoBektesevic]

The way this reads to me is as if you want me to make optional keyword arguments just keyword arguments - in which case every invocation of the method will require the user to provide them and that sounds wrong? Alternative understanding is to user **kwargs but that sounds wrong too? Could you clarify what you meant?

[bsipocz]

A lot of nitpicks, with a few genuine fixes to be made, mostly around the tests, but all of them are very straightforward.

And this needs a changelog entry.

Also, something looks weird with the rendered API docs in the most.rst page, but it all renders well in the one layer up namespace.

[bsipocz]

this is good for now, but I wonder whether we can/should loosen this up a bit, elsewhere we mostly are case insensitive.
(though it probably should be an upstream fix if upstream is this sensitive to the case)

[DinoBektesevic]

For future reference

================================================================================
 
Toolkit version: N0065
 
SPICE(SPKINSUFFDATA) --
 
Insufficient ephemeris data has been loaded to compute the state of -99999
relative to 0 (SOLAR SYSTEM BARYCENTER) at the ephemeris epoch 2010 JAN 07
00:01:06.184.
 
A traceback follows.  The name of the highest level module is first.
spkez_c --> SPKEZ --> SPKACS --> SPKAPS --> SPKLTC --> SPKGEO
 
================================================================================

are the kinds of errors that pop up if they are not first-letter-capitalized. The upstream is basically just a form whose payload is directly delivered to SPICE toolkit and their internal search engine, optionally some additional bespoke tools.

We can fix this, as well as rename some of these parameters to be more consistent in the qparams map. Why arg_perihelion but perih_dist and perih_time? Shouldn't it all be short or all of it long? Same with the input_type and output_mode - an arbitrary change that makes it, in my humble opinion, much harder to remember how to use the thing. Most of the obj_ prefixes could be dropped from the parameter's name too. If this is getting passed onward, something to think about.

[bsipocz]

I've added one more commit that makes all remote doctests pass on my laptop. We'll likely need to add a few more IGNORE_OUTPUTS (currently only added it for those that print out the temporary workspace url, that changes for each query).
If tests keep failing on the cron CI, but for now it all looks fine.

[bsipocz]

Thank you so much @DinoBektesevic, and I'm sorry for dragging it along for so long.