New ephemeris generation via command-line script #396
Conversation
|
Hello @mkelley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-11-12 01:18:51 UTC |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #396 +/- ##
==========================================
+ Coverage 83.15% 84.59% +1.44%
==========================================
Files 88 88
Lines 8191 8392 +201
==========================================
+ Hits 6811 7099 +288
+ Misses 1380 1293 -87 ☔ View full report in Codecov by Sentry. |
mkelley
force-pushed
the
ephemeris-cli-script
branch
from
fce55ec
to
f038c77
Compare
|
Consider some refactoring to align with https://docs.astropy.org/en/latest/development/scripts.html |
mkelley
force-pushed
the
ephemeris-cli-script
branch
4 times, most recently
from
4309454
to
d05b0f0
Compare
|
Got it to run. Thanks @mkelley! Generally works fine for me. The one thing I note that it does not fail very gracefully, i.e., anything that causes a query failure shows a traceback of the sbpy code + the error message from the query service. These would include errors like an end date that's before the start date or an unknown object designation (the two I've tried so far after searching for "433P" with all default settings failed). If this is the desired outcome, then that's fine of course (probably the most useful for the user being able to understand what's wrong with their query and fix it, but it maybe ideally should be made clear that the problem is with their query, and not the code). If not though, I wonder if there's some simple error handling that could be added (e.g., returning a simplified version of the query service error message and without the sbpy code traceback, since it's not really relevant to the user error). Let me know what you think though. If this is beyond the scope of what you were planning for this, I'm happy to just send it through and make it clear that it's the user's responsibility to submit a valid query, and/or create a new issue/PR to add error handling later so that this can get added and used in its current basic form assuming perfect user usage. |
|
The help text could also be a bit more descriptive for the time parameters, e.g., the accepted formats for the start and stop time, and how to express time step units (looks like days and hours are abbreviated as d and h, but minutes are denoted as min). Could maybe specify observer location is the MPC observatory code (unless there are other ways to specify location, like latitude/longitude). Also, is there an easy way to set an airmass/elevation limit and/or daytime exclusion (if not, no problem, since the documentation does say only limited options are available)? |
|
Thanks. I'll work on those points. Error handling can be improved. Maybe a default of just reporting the error message and a --debug mode to show the full trace? |
|
Yes, that sounds good. |
mkelley
force-pushed
the
ephemeris-cli-script
branch
from
ba92e35
to
a86e2b2
Compare
I'd like to keep it simple, so for now can we leave this out? |
|
This is ready for a re-review. Check out the new error handling. It should be less ugly. |
|
Sorry for the delay...finally got a chance to take the new version out for a spin...some of the error handling definitely looks better. I still see a few remaining issues:
I can raise errors doing other things (e.g., miriade doesn't seem to have comets), but I think these can be left to the user to figure out on their own. The issues above (mainly 1 and 2, where 3 can just be an edit of the error message, and 4 is not a big deal either way) are the only ones I think actually need action before pushing this ahead. |
Yeah, that's not the best. But do we fix that here or in astroquery? I guess we could strip html tags?
Oops, oorb is not supposed to be there! I'll fix that.
Then this should be added to astroquery, where we already add the current message ("Maybe try different id type?").
Good point. It is intended to be the number of steps, and I think this is clear in the help for the parameter. Maybe it could be renamed --steps, but we already have a --step parameter. Changing the behavior to number of ephemeris rows would break the alignment with the number parameter in the Ephem.from_* methods. Would it be OK to leave this as is?
For miriade, try --type=comet. I think this is documented in the help. |
Update documentation. Better column normalization. Remove ability to run module via __main__ to avoid package import warning. Rewrite as a class; add tests. Update docs Restore requested and returned targets in table comments. Merge target identifier and target name The two were already confused: "target name" was in "target identifier", but "targetname" was a separate field. Reformat file Missing self Save some basic metadata in Ephem tables. Mock remote horizons comet test. Improve astroquery mocking, and mock more tests. Codestyle; Require astroquery for test Restore failing C/1995 O1 test. Mark code block with bash in docs. Split a string to decrease line length. Revert "Save some basic metadata in Ephem tables." This reverts commit b6429ab. Remove meta lines again.
mkelley
force-pushed
the
ephemeris-cli-script
branch
from
577c736
to
ce223d5
Compare
|
I'd say that stripping HTML tags would probably be good enough for unknown objects for MPC queries if it's not too much trouble. Otherwise, it can stay as-is. I think everything else can likewise stay as-is since they're minor and/or external to sbpy |
A "simple" program to show the ephemeris of an object, with options for Horizons, Minor Planet Center, and Miriade. sbpy installation now installs a script named
sbpy-ephem.Some examples:
MPC
Horizons
Miriade