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

ENH: querying tool for the Planetary Ring Node #2358

Merged
merged 25 commits into from
Jan 5, 2023
Merged

ENH: querying tool for the Planetary Ring Node #2358

merged 25 commits into from
Jan 5, 2023

Conversation

emolter
Copy link
Contributor

@emolter emolter commented Apr 8, 2022

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

@pep8speaks
Copy link

pep8speaks commented Apr 8, 2022

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! 🍻

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

@codecov
Copy link

codecov bot commented Apr 12, 2022

Codecov Report

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

❗ 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%> (ø)

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

Copy link
Contributor

@mkelley mkelley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emolter , this generally looks good. Thanks for the contribution so-far. I've looked at the top ~half of the code and have some suggestions. I have not yet run it, but instead just providing some feedback on style at this time.

<https://pds-rings.seti.org/tools/>


# basic query for all six targets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please place the example in a section:

Examples
--------
...

----------
planet : str, required. one of Jupiter, Saturn, Uranus, or Neptune
Name, number, or designation of the object to be queried.
obs_time : str, in JD or MJD format. If no obs_time is provided, the current
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time should use an astropy Time object. This will facilitate transformations between time formats and scales.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it ok/good to make it so that either an astropy Time object or a string can be passed? and then within the code if a string is passed then it becomes a Time object?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is OK, but you will need to document the time scale assumed.


def __str__(self):
"""
String representation of RingNodeClass object instance'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing single quote character


super().__init__()
self.planet = planet
self.obs_time = obs_time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is a reason to reuse planet and obs_time, or if you have a strong preference, I prefer to have everything happen in a single method call, e.g., RingNode.ephemeris(planet, obs_time, observer, ...).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, cool, I like this idea too. the only reason I did it the way I did was to look more like the jplhorizons module

>>> from astroquery.solarsystem.pds import RingNode
>>> uranus = RingNode(planet='Uranus',
... obs_time='2017-01-01 00:00')
>>> eph = obj.ephemeris() # doctest: +SKIP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By assigning RingNode(...) to uranus, this example becomes a little bit muddied. I would think that obs_time is more closely aligned with the ephemeris() parameters, rather than the planet itself.

Parameters
----------
self : RingNodeClass instance
observer_coords : three-element list/array/tuple of format (lat (deg), lon (deg east), altitude (m))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using astropy Quantity or Angle to take care of the units. See MPC.get_ephemeris() where I also allowed astropy.coordinates.EarthLocation, but I'll leave it up to you if you want to implement that.

"ephem",
conf.planet_defaults[self.planet]["ephem"],
), # change hardcoding for other planets
("time", self.obs_time),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What time scale is used here? UTC? TDB?

astroquery/solarsystem/pds/core.py Show resolved Hide resolved

# Horizons has machinery here to mock request from a text file
# is that necessary? why is that done?
# wouldn't we want to know if the website we are querying changes something that makes code fail?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remote tests are not routinely run. With a mock test, we can run many tests in a short period of time without concern for server load or even internet connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll do my best with offline tests

@emolter
Copy link
Contributor Author

emolter commented May 6, 2022

@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
Copy link
Contributor

mkelley commented May 6, 2022

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
Copy link
Contributor

mkelley commented May 6, 2022

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
Copy link
Contributor Author

emolter commented May 9, 2022

@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
Copy link
Member

bsipocz commented May 10, 2022

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.

@bsipocz bsipocz added this to the v0.4.7 milestone May 10, 2022
@mkelley
Copy link
Contributor

mkelley commented May 13, 2022

Hi @emolter ,

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

@mkelley
Copy link
Contributor

mkelley commented May 13, 2022

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

@emolter emolter marked this pull request as ready for review May 13, 2022 22:03
@emolter
Copy link
Contributor Author

emolter commented May 13, 2022

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
Copy link
Member

bsipocz commented May 16, 2022

@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
Copy link
Contributor

mkelley commented May 18, 2022

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
Copy link
Contributor Author

emolter commented May 20, 2022

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
Copy link
Contributor

mkelley commented May 20, 2022

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'
>>> 

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.

A lot of comments, many of them are nitpicks, some are for code clean-ups.
I think is very close to be being ready, but could benefit some simplification.

astroquery/solarsystem/pds/core.py Outdated Show resolved Hide resolved
astroquery/solarsystem/pds/core.py Outdated Show resolved Hide resolved
astroquery/solarsystem/pds/core.py Outdated Show resolved Hide resolved
astroquery/solarsystem/pds/core.py Outdated Show resolved Hide resolved
astroquery/solarsystem/pds/core.py Outdated Show resolved Hide resolved
astroquery/solarsystem/pds/tests/test_ringnode.py Outdated Show resolved Hide resolved
docs/solarsystem/pds/pds.rst Outdated Show resolved Hide resolved
docs/solarsystem/pds/pds.rst Outdated Show resolved Hide resolved
docs/solarsystem/pds/pds.rst Outdated Show resolved Hide resolved
systemtable, bodytable, ringtable = self._parse_ringnode(response.text)
except Exception as ex:
try:
self._last_query.remove_cache_file(self.cache_location)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you do this? To disable caching remove the kwarg instead and pass cache=False to self._request.

Copy link
Contributor

@mkelley mkelley May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this block trying to avoid caching a failed query or bad response?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we get to the bottom of the cache invalidation PR at some point, until then if modules wish to do this, they should also include it in the test mocks.

Also, I would say no Exception should be caught this widely but should be more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly I have no idea how to do this "right," I pretty much copied everything here from the JPL Horizons query. May I request some help here? I'm happy with however this ends up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's come back to this once you push the updates. Without fixing it, at least in the mocks, the test will fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I was following the SIMBAD example 😄 😵‍💫 I'll look at a few updates to jplhorizons now.

Copy link
Contributor Author

@emolter emolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for the reviews @bsipocz @mkelley @eerovaher ! I went through all of the comments and made as many changes as I could. I'm about to commit those changes so you can look at what I did. I have some outstanding questions - some stuff went over my head given that this is my first ever pull request. I added comments inside the review as well, but here are the high-level things:

  1. is there a way for me to build and view the documentation locally, so I can see if all the linking works properly?
  2. 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?
  3. 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?

@bsipocz
Copy link
Member

bsipocz commented May 24, 2022

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
Copy link
Member

bsipocz commented May 24, 2022

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
Copy link
Member

bsipocz commented May 24, 2022

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
Copy link
Contributor Author

emolter commented May 24, 2022

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: #2358 (comment)
and this: #2358 (comment)
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
Copy link
Member

bsipocz commented May 24, 2022

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
Copy link
Contributor Author

emolter commented May 24, 2022

I still didn't know what to do with #2358 (comment) so that might still need looking at

@bsipocz
Copy link
Member

bsipocz commented Jan 5, 2023

OK, so I think we kept this open long enough. I have addressed the last set of comments, but as there has been multiple rounds of many suggestions I could easily missed a few. Anyway, tests all pass and the module is functional, so any further improvements can be done in a follow-up PR.

@bsipocz
Copy link
Member

bsipocz commented Jan 5, 2023

Thanks @emolter!

@bsipocz bsipocz merged commit af87c33 into astropy:main Jan 5, 2023
@emolter emolter deleted the ringnode branch January 7, 2023 06:56
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.

5 participants