ENH: querying tool for the Planetary Ring Node #2358
Conversation
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
@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.
astroquery/solarsystem/pds/core.py
Outdated
| <https://pds-rings.seti.org/tools/> | ||
|
|
||
|
|
||
| # basic query for all six targets |
There was a problem hiding this comment.
Please place the example in a section:
Examples
--------
...
astroquery/solarsystem/pds/core.py
Outdated
| ---------- | ||
| 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 |
There was a problem hiding this comment.
Time should use an astropy Time object. This will facilitate transformations between time formats and scales.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think that is OK, but you will need to document the time scale assumed.
astroquery/solarsystem/pds/core.py
Outdated
|
|
||
| def __str__(self): | ||
| """ | ||
| String representation of RingNodeClass object instance' |
There was a problem hiding this comment.
trailing single quote character
astroquery/solarsystem/pds/core.py
Outdated
|
|
||
| super().__init__() | ||
| self.planet = planet | ||
| self.obs_time = obs_time |
There was a problem hiding this comment.
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, ...).
There was a problem hiding this comment.
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
astroquery/solarsystem/pds/core.py
Outdated
| >>> from astroquery.solarsystem.pds import RingNode | ||
| >>> uranus = RingNode(planet='Uranus', | ||
| ... obs_time='2017-01-01 00:00') | ||
| >>> eph = obj.ephemeris() # doctest: +SKIP |
There was a problem hiding this comment.
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.
astroquery/solarsystem/pds/core.py
Outdated
| Parameters | ||
| ---------- | ||
| self : RingNodeClass instance | ||
| observer_coords : three-element list/array/tuple of format (lat (deg), lon (deg east), altitude (m)) |
There was a problem hiding this comment.
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.
astroquery/solarsystem/pds/core.py
Outdated
| "ephem", | ||
| conf.planet_defaults[self.planet]["ephem"], | ||
| ), # change hardcoding for other planets | ||
| ("time", self.obs_time), |
There was a problem hiding this comment.
What time scale is used here? UTC? TDB?
|
|
||
| # 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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok, I'll do my best with offline tests
|
@mkelley thanks very much for the helpful comments! I had a few specific questions as well, if you don't mind.
Thanks again. I'm going to work on this some more in the next week or so and see how far I can get |
|
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=htmlUse py39 for python 3.9, or py38 for 3.8, etc. This will only test the pds submodule. Append |
|
Regarding the output, I think three return variables is fine, and leaving |
|
@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:
Any advice on these two issues is much appreciated! |
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. |
|
Hi @emolter , Were you able to fix the mock tests? The tests seem to be passing now. |
|
Also, the file coverage.xml seems to have been added by mistake. Please remove it. |
|
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. |
|
@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. |
|
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. |
|
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? |
|
Perhaps you can annotate the table with a loop, adding the units to the >>> 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'
>>> |
astroquery/solarsystem/pds/core.py
Outdated
| systemtable, bodytable, ringtable = self._parse_ringnode(response.text) | ||
| except Exception as ex: | ||
| try: | ||
| self._last_query.remove_cache_file(self.cache_location) |
There was a problem hiding this comment.
Why do you do this? To disable caching remove the kwarg instead and pass cache=False to self._request.
There was a problem hiding this comment.
Isn't this block trying to avoid caching a failed query or bad response?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
OK, let's come back to this once you push the updates. Without fixing it, at least in the mocks, the test will fail.
There was a problem hiding this comment.
And I was following the SIMBAD example 😄 😵💫 I'll look at a few updates to jplhorizons now.
There was a problem hiding this comment.
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:
- is there a way for me to build and view the documentation locally, so I can see if all the linking works properly?
- 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?
- 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. |
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. |
|
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. |
|
I still didn't know what to do with #2358 (comment) so that might still need looking at |
…ute '_last_query'
…endent newline handling
|
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. |
|
Thanks @emolter! |
The goal is to query the Planetary Ring Node's ephemeris tools at https://pds-rings.seti.org/tools/