Draft of querying tool for the Planetary Ring Node

[emolter]

The goal is to query the Planetary Ring Node's ephemeris tools at https://pds-rings.seti.org/tools/

[pep8speaks]

Hello @emolter! 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! :beers:

Comment last updated at 2022-09-06 16:55:35 UTC

[codecov[bot]]

Codecov Report

Merging #2358 (609ad39) into main (39638db) will increase coverage by 0.24%. The diff coverage is 90.40%.

:exclamation: Current head 609ad39 differs from pull request most recent head 51ead5f. Consider uploading reports for the commit 51ead5f to get more accurate results

@@            Coverage Diff             @@
##             main    #2358      +/-   ##
==========================================
+ Coverage   68.68%   68.92%   +0.24%     
==========================================
  Files         294      299       +5     
  Lines       22292    22542     +250     
==========================================
+ Hits        15311    15537     +226     
- Misses       6981     7005      +24     

Impacted Files	Coverage Δ
...stroquery/solarsystem/pds/tests/test_pds_remote.py	41.17% <41.17%> (ø)
astroquery/solarsystem/pds/tests/setup_package.py	50.00% <50.00%> (ø)
astroquery/solarsystem/pds/core.py	91.78% <91.78%> (ø)
astroquery/solarsystem/pds/__init__.py	100.00% <100.00%> (ø)
astroquery/solarsystem/pds/tests/test_pds.py	100.00% <100.00%> (ø)

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

[emolter]

@mkelley thanks very much for the helpful comments! I had a few specific questions as well, if you don't mind.

1. the output of this code is three different groups of data: system-wide parameters systemtable, target-specific moon data bodytable, and target-specific ring data ringtable. Tthe bodytable and ringtable make sense as astropy Tables: there are multiple moons (rows) that have the same parameters (columns) associated with them. But the systemtable is dict-like: it makes sense to be able to call, for example, systemtable['opening_angle'] and return a single Quantity object. My question is: what astropy data structure should I use for something dict-like? And more broadly, is it okay to have three different tables returned from the query this way, or is there some higher-level data structure this stuff should go into?
2. what exactly are the testing requirements? is there any avenue for me to ask for help writing tests once the code operates in a good way?
3. I assume I need to use QTables instead of Tables when I have units - is that correct? sorry I'm so unfamiliar with Astropy generally...

Thanks again. I'm going to work on this some more in the next week or so and see how far I can get

[mkelley]

Regarding offline testing, there are some notes at: https://astroquery.readthedocs.io/en/latest/testing.html Another benefit to offline tests is that the output from the "server" is always the same (i.e., the ephemeris never changes). But definitely have a remote test or two so that we can address unexpected breaking format changes from the Node.

Regarding testing in general, you'll want to exercise and validate every line of code. The output from codecov should help with that (it currently reads 16% covered), or you can run the tests offline and get a coverage report. I would try using tox if you can, e.g.,:

tox -e py39-test-cov -- -Psolarsystem.pds --cov-report=html

Use py39 for python 3.9, or py38 for 3.8, etc. This will only test the pds submodule. Append --remote-tests to also run remote tests. Then open .tmp/py38-test-cov/htmlcov/index.html and navigate to your submodule's files to see what was covered. For more help, either send me a slack message or reply in this PR.

[mkelley]

Regarding the output, I think three return variables is fine, and leaving systemtable as a dictionary is also OK. As for Table vs. QTable, yeah, try out QTable first.

[emolter]

@mkelley ok, I incorporated all of your comments - I put astropy units onto all the input and output quantities, including your suggestion to allow for an EarthLocation structure to be passed. I'm now running into a few small problems:

1. Do you have a suggested automatic de-linter? The ones I'm using (autopep8, black) don't seem to fix everything, and sometimes make style changes that are incompatible with what tox is expecting (i.e., they seemingly make things worse!)
2. The offline test_ephemeris_query works on my machine (using the tox command you suggested above) but is failing to find the data files (tests/data/uranus_ephemeris.html) when the CI workflow is run.

Any advice on these two issues is much appreciated!

[bsipocz]

Do you have a suggested automatic de-linter? The ones I'm using (autopep8, black) don't seem to fix everything, and sometimes make style changes that are incompatible with what tox is expecting (i.e., they seemingly make things worse!)

please don't use black. autopep8 should work for most things, for the rest, I would suggest to set up flake to run in your editor, that will highlight potential pep8 issues while you develop the code.

[mkelley]

Hi @emolter ,

Were you able to fix the mock tests? The tests seem to be passing now.

[mkelley]

Also, the file coverage.xml seems to have been added by mistake. Please remove it.

[emolter]

Ok, I fixed everything you suggested @mkelley , thanks again for all your help! I think this is nearly complete now. I added some documentation and some more tests. What is the next step at this stage? What additional feedback do you have?

Could you please help me understand why the oldest dependencies and codecov/project checks are failing? The error messages are hard for me to parse.

[bsipocz]

@emolter - I'll try to get back to this PR later this week for a round of review and suggestions for fixing the code to pass CI.

[mkelley]

The "codecov/patch` details list all the lines that were not tested, e.g., astroquery/solarsystem/pds/core.py#L50 states that line 50 of that file was not covered by the tests. I verified this locally and even with the remote tests enabled, the results were similar. Most of the skipped lines are due to if-statement branching, e.g., lines 411-418 and 426-430 were not covered by the tests. So, having Saturn and Neptune queries in the tests seems important.

[emolter]

Ok, I added tests for the special cases for Saturn and Neptune. It looks like now the oldest dependencies checks are failing because QTable doesn't like the "units" keyword. But again all the tests pass for me with tox, even in python 3.7. If I am correctly interpreting the error messages and this is indeed the problem, can you please advise me how to add units to a table that was read from ascii in a way that makes the oldest dependencies happy?

[mkelley]

Perhaps you can annotate the table with a loop, adding the units to the Column.unit properties?

>>> from astropy.table import Table, QTable
>>> tab = Table(rows=[{'a': 1}])
>>> tab
<Table length=1>
  a  
int64
-----
    1
>>> tab['a'].unit = 'km'
>>> tab
<Table length=1>
  a  
  km 
int64
-----
    1
>>> QTable(tab)
<QTable length=1>
   a   
   km  
float64
-------
    1.0
>>> QTable(tab)['a']
<Quantity [1.] km>
>>> import astropy
>>> astropy.__version__
'4.0'
>>> 

[bsipocz]

is there a way for me to build and view the documentation locally, so I can see if all the linking works properly?

python setup.py build_sphinx will build the docs. It should come back without any warnings

[bsipocz]

Related, @bsipocz suggested removing self.uri and the return_raw flag, among other things. I don't really understand the benefits/drawbacks of doing this - TBH I just copy-pasted what was in the Horizons query tool. Can someone please advise?

imo they should be removed from horizons, too. As I mentioned it's likely slipped through the original code review. Basically all that info is available with the async method, no need to duplicate and store it on the instance.

[bsipocz]

It sounded like there was some debate over how exactly to do the request handling. Would someone be willing to just make the edits they see fit?

I lost track what the debate was, could you cross link. If this is the only thing standing I can fit it up before merging.

[emolter]

It sounded like there was some debate over how exactly to do the request handling. Would someone be willing to just make the edits they see fit?

I lost track what the debate was, could you cross link. If this is the only thing standing I can fit it up before merging.

sorry, I should have been more specific. I was confused about this: https://github.com/astropy/astroquery/pull/2358#discussion_r879062838 and this: https://github.com/astropy/astroquery/pull/2358#discussion_r879638332 I think I answered the first one myself, I definitely should go ahead and merge _parse_result with _parse_ringnode. But the second one, I am not really sure what to do there

[bsipocz]

Ahh, OK, for the second one, let's see what the failure will say, we may need to patch your MockResponse in the test, but leave the code as is, until we have a fixed cache mechanism. That part as I now recall was to workaround some failures users came by picking up outdated cached queries for jplhorizons.

[emolter]

I still didn't know what to do with https://github.com/astropy/astroquery/pull/2358#discussion_r880860547 so that might still need looking at

[emolter]

Is there a back-compatible way to rewrite epoch.utc.to_value("iso", subfmt="date_hm"))? This passes the tests with tox on my machine but fails here.

[mkelley]

How about parsing the string? Time.now().utc.iso[:16]

[bsipocz]

One more nitpick: please git mv the test files from ringnode to be pds.

[bsipocz]

OK, so the big difference compared to the jplhorizons example is that they don't hit the part of the code where _last_query is being used, so it's enough for that module to patch only one of the test functions.

There are two ways to tackle this:

• use an explicit fresh instance in each tests. you basically do this, but don't patch its last_query with the dummy. So your typical test function will start with the following, and then run the methods of this instance.

pds_inst = pds.RingNode()
pds_inst._last_query = AstroQuery('GET', 'http://dummy')

• create a patched instance and use it as a fixture. e.g. alma does this for the remote tests

[emolter]

@bsipocz thanks for your continued help with this! I finally made some more progress - sorry for the delay. But now the "oldest dependencies" checks are failing due to an error I don't understand, something to do with how I'm instantiating the astropy tables... would you please be able to shed some light on what's happening there? once again, this succeeds on my machine using tox and py3.7

[emolter]

@eerovaher @mkelley @bsipocz Thanks again for your help with this piece of code! Is it still possible to get this merged for 5.2? if so, what is needed from me to make that happen?

[eerovaher]

astroquery release schedule is independent from astropy.

[bsipocz]

OK, so first of all, I go ahead and rebase this to pick up more recent changes of CI, then I'll do a final review.

[bsipocz]

@emolter - thanks for following up on the review. I have have a detailed look on the remaining windows issues later this evening and eventually get this merged!

[emolter]

Ok @bsipocz, thanks again so much for your helpful comments on this piece of code. I removed all explicit calls to \n, preferring splitlines(), changing split(' \n') to simply split(), and when absolutely necessary using os.linesep. I changed all the other things you suggested; at least, I think I did. It's now passing again.

Sorry if these are basic questions, but (1) how/where do I write a changelog entry and what is required, and (2) how do I squash commits into logical chunks?