Add `vec_table` as an API Parameter for JPL Horizons

[megargayu]

The JPL Horizons module is missing some API parameters that are allowed by NASA, including the vec_table parameter. This is especially important for uncertainties of certain objects (like small asteroids), and this pull request aims to add this functionality into astroquery. See here for the reference.

However, because of the way that JPL Horizons names columns, there can be naming conflicts between columns with the correct input (for example, vec_table="2xarp" would cause many conflicts, like R_s, A_s, etc; see changes in astroquery/jplhorizons/__init__.py for more information). Whenever this happens, the program fails with an error. (Supporting this is not strictly required, I assume, as all of these values are usually not needed in the same request).

Is this an intrinsic limitation of astropy's implementation of tables, or is there a way to fix this error?

[keflavich]

Could you provide more specific examples? e.g., show a request that returns a table (in html, json, votable, or whatever form) that has conflicting column names?

I don't think there is an intrinsic limitation here; if there are conflicting column names, they'll have _1, _2, etc. appended, afaik. But I'm sure there's a workaround.

[bsipocz]

cc @mkelley

[megargayu]

Could you provide more specific examples? e.g., show a request that returns a table (in html, json, votable, or whatever form) that has conflicting column names?

I don't think there is an intrinsic limitation here; if there are conflicting column names, they'll have _1, _2, etc. appended, afaik. But I'm sure there's a workaround.

Try running with the following code:

from astroquery.jplhorizons import Horizons

obj = Horizons(
  id="Ceres",
  location="500@399",
)

vectors = obj.vectors(
  aberrations="apparent",
  vec_table="2xarp",
)

print(vectors)

Or look at a corresponding API request here and scroll to the table. The following error occurs with the code:

astropy.io.ascii.core.InconsistentTableError:
ERROR: Unable to guess table format with the guesses listed below:
reader_cls:Ecsv fast_reader: {'enable': False} fill_values: [('.n.a.', '0'), ('n.a.', '0')] names: ['JDTDB', 'Calendar Date (TDB)', 'X', 'Y', 'Z', 'VX', 'VY', 'VZ', 'X_s', 'Y_s', 'Z_s', 'VX_s', 'VY_s', 'VZ_s', 'A_s', 'C_s', 'N_s', 'VA_s', 'VC_s', 'VN_s', 'R_s', 'T_s', 'N_s', 'VR_s', 'VT_s', 'VN_s', 'A_s', 'D_s', 'R_s', 'VA_RA_s', 'VD_DEC_s', 'VR_s', '_dump'] strict_names: True
reader_cls:FixedWidthTwoLine fast_reader: {'enable': False} fill_values: [('.n.a.', '0'), ('n.a.', '0')] names: ['JDTDB', 'Calendar Date (TDB)', 'X', 'Y', 'Z', 'VX', 'VY', 'VZ', 'X_s', 'Y_s', 'Z_s', 'VX_s', 'VY_s', 'VZ_s', 'A_s', 'C_s', 'N_s', 'VA_s', 'VC_s', 'VN_s', 'R_s', 'T_s', 'N_s', 'VR_s', 'VT_s', 'VN_s', 'A_s', 'D_s', 'R_s', 'VA_RA_s', 'VD_DEC_s', 'VR_s', '_dump'] strict_names: True
reader_cls:RST fast_reader: {'enable': False} fill_values: [('.n.a.', '0'), ('n.a.', '0')] names: ['JDTDB', 'Calendar Date (TDB)', 'X', 'Y', 'Z', 'VX', 'VY', 'VZ', 'X_s', 'Y_s', 'Z_s', 'VX_s', 'VY_s', 'VZ_s', 'A_s', 'C_s', 'N_s', 'VA_s', 'VC_s', 'VN_s', 'R_s', 'T_s', 'N_s', 'VR_s', 'VT_s', 'VN_s', 'A_s', 'D_s', 'R_s', 'VA_RA_s', 'VD_DEC_s', 'VR_s', '_dump'] strict_names: True

And so on with many readers until it ends with:

************************************************************************
** ERROR: Unable to guess table format with the guesses listed above. **
**                                                                    **
** To figure out why the table did not read, use guess=False and      **
** fast_reader=False, along with any appropriate arguments to read(). **
** In particular specify the format and any known attributes like the **
** delimiter.                                                         **
************************************************************************

This only happens (I assume) because, for example, A_s is repeated twice (look at the error message and the table on the API link). It most likely is a limitation with the reader astropy uses. If you try something like vec_table="2x", vec_table="2a", or any combination that doesn't cause naming conflicts in columns, then the code works perfectly fine.

[keflavich]

Yep, that's what's happening, but it is a pretty easy fix. e.g., after:

        headerline = [h.strip() for h in headerline]

just add something like:

duplicates = {hl for hl in headerline if headerline.count(hl) > 1}
dup_inds = {dup: [ii for ii, hl in enumerate(headerline) if hl == dup] for dup in duplicates}
for hl, inds in dup_inds.items():
    for ii, ind in enumerate(inds):
        headerline[ind] = f'{hl}_{ii}'

...which admittedly took some mental hoop jumping to come up with, but I think it'll work

[megargayu]

Yep, that's what's happening, but it is a pretty easy fix. e.g., after:

        headerline = [h.strip() for h in headerline]

just add something like:

duplicates = {hl for hl in headerline if headerline.count(hl) > 1}
dup_inds = {dup: [ii for ii, hl in enumerate(headerline) if hl == dup] for dup in duplicates}
for hl, inds in dup_inds.items():
    for ii, ind in enumerate(inds):
        headerline[ind] = f'{hl}_{ii}'

...which admittedly took some mental hoop jumping to come up with, but I think it'll work

I just committed some code based on this (a bit more readable) that should work. I also had to change downstream code so it would work with the duplicate column names, namely the ones reading in unit data & renaming columns based on __init__.py. Now, the code doesn't throw an error with vec_table="2xarp" and any other combinations I've tried, so it looks like it works!

[mkelley]

This looks like a good addition, thanks! Let me know if you have any questions about fixing the tests. It sounds like it would be useful to design a new test that specifically exercises this column renaming.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.87%. Comparing base (a4357f6) to head (0e15737). Report is 80 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/jplhorizons/core.py	71.42%	6 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3273      +/-   ##
==========================================
+ Coverage   69.09%   69.87%   +0.78%     
==========================================
  Files         232      232              
  Lines       19677    19757      +80     
==========================================
+ Hits        13595    13805     +210     
+ Misses       6082     5952     -130     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

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

[megargayu]

This looks like a good addition, thanks! Let me know if you have any questions about fixing the tests. It sounds like it would be useful to design a new test that specifically exercises this column renaming.

Added a new test (test_vectors_query_two) for the new parameter that passes. Also, I'm not sure why, but it seems like the following tests are failing for jplhorizons on the current main branch of astroquery without any of my changes:

FAILED astroquery/jplhorizons/core.py::astroquery.jplhorizons.core.HorizonsClass.vectors_async
FAILED astroquery/jplhorizons/tests/test_jplhorizons_remote.py::TestHorizonsClass::test_ephemerides_query - AssertionError: assert array([False])
FAILED docs/jplhorizons/jplhorizons.rst::jplhorizons.rst

Also, should the docs be changed for installing in "editable" mode with the following pip install instead?

pip install -e .[all,test,docs] --config-settings editable_mode=strict

The editable_mode=strict was necessary to get the install working, and I had to search around a lot until I found the correct solution. It could be very helpful for new contributors to have this config option in the actual documentation at https://astroquery.readthedocs.io/en/latest/#building-from-source.

Apart from those separate issues/things to look into, the test now works!

[bsipocz]

The editable_mode=strict was necessary to get the install working, and I had to search around a lot until I found the correct solution. It could be very helpful for new contributors to have this config option in the actual documentation at

Could you elaborate on this in a separate issue? I never had any problems with the editable installs even without this extra option.

[megargayu]

The editable_mode=strict was necessary to get the install working, and I had to search around a lot until I found the correct solution. It could be very helpful for new contributors to have this config option in the actual documentation at

Could you elaborate on this in a separate issue? I never had any problems with the editable installs even without this extra option.

I actually figured out what the issue was - if you install in editable mode, and you try to from astroquery.jplhorizons import Horizons from a file outside of the cloned astroquery github directory (so the parent folder has venv, astroquery, and test.py in it), the following error is raised:

Traceback (most recent call last):
  File "[...]\test.py", line 1, in <module>
    from astroquery.jplhorizons import Horizons
  File "[...]\astroquery\astroquery\jplhorizons\__init__.py", line 240, in <module>
    from .core import Horizons, HorizonsClass
  File "[...]\editable_test\astroquery\astroquery\jplhorizons\core.py", line 23, in <module>
    from ..query import BaseQuery
  File "[...]\editable_test\astroquery\astroquery\query.py", line 26, in <module>
    from astroquery import version, log, cache_conf
ImportError: cannot import name 'log' from 'astroquery' (unknown location)

However, this is fixed if you put test.py inside the cloned astroquery github repository, and run it with the astroquery module as the current working directory. So it's an overall small issue that only comes up if you test something with an external script.

[larshinueber]

Hi @megargayu, Thank you for contributing the feature, this was exactly what I was looking for! I've tested your implementation locally and it's been working great. Let me know if there is anything I can help with to get this merged, would love to see this in a stable release :)