-
Notifications
You must be signed in to change notification settings - Fork 34
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
Extended packed designation functionality #406
Extended packed designation functionality #406
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
==========================================
+ Coverage 82.99% 83.19% +0.20%
==========================================
Files 88 88
Lines 8109 8183 +74
==========================================
+ Hits 6730 6808 +78
+ Misses 1379 1375 -4 ☔ View full report in Codecov by Sentry. |
I'll do a detailed review later, but it looks like two of your lines are not covered by the tests: https://app.codecov.io/gh/NASA-Planetary-Science/sbpy/pull/406/blob/sbpy/data/names.py#L164 Can you add a case that raises IndexError or ValueError? You can test it like so: def test_raises_error():
with pytest.raises(IndexError):
Names.to_packed("bad_input_here") There are a couple other blocks not in our testing suite, but they don't seem to be related to your edits, so I won't make you cover them. |
Just a note, I think line 140 (which I didn't write, but just put inside a try-except block; but which is currently flagged as uncovered code) is extraneous, since the "s[6:].isdigit()" condition above it should already prevent anything that looks like "1995 A" (i.e., single letter, no subscript) from being passed through that section. As such, I think it can be deleted, but just wanted to get a second opinion before doing so. |
You're right, except that we can get to that line with a cometary fragment style designation. I'm seeing two problems.
They are unrelated, so we should proceed with this PR, and open issues for each of them. |
Oh yeah, I did notice that multi-letter fragments were not accounted for in the code, but also that they do not seem to be accounted for by the MPC's own documentation (https://www.minorplanetcenter.net/iau/info/PackedDes.html), except by making the 7 character packed designation into an 8 character designation since the fragment is simply recorded in plain-text. And yeah, 1234 A-A seems like a different issue, so maybe better to treat it with a separate issue/PR? |
By the way, just for traceability, in working on the code for the extended packed provisional designations, I found that the MPC had also extended its scheme for packed permanent designations beyond 620000, so code for handling extended packed permanent designations is also included in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updates look good. Since you tagged it v0.6, we will merge after v0.5 is out.
sbpy/data/names.py
Outdated
and len(p) == 7 | ||
): | ||
# packed numbers translation string with no I | ||
pkd_noI = 'ABCDEFGHJKLMNOPQRSTUVWXYZ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is not used and should be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to rebase with the updated main branch, then resolve the changes.rst conflicts. Let me know if you want some tips.
Hm, yeah, can you send me info on how to do that? Thanks! |
20ae799
to
037c6de
Compare
removed second instance of pkd_noI definition fixed test Update test_names.py Update test_names.py changed indentation of a closing ) fixed error with unpacking permanent desigs fixed typo modified code structure slightly + added tests removed test_break_packed() case that is no longer invalid fixed typo in test to_packed() statements added quotes to numerical designations in test statements fixed typos added functionality for extended permanent asteroid designations continuing to try to cover code with tests Update test_names.py Update test_names.py added another test error added different test errors Update test_names.py Update test_names.py error not being raised...testing more testing error raising removed example that didn't raise error changed ValueError and IndexError to TargetNameParseError changed some ValueError examples to IndexError added ValueError examples to test_names.py added ValueError examples to test_names.py fixed typo fixed typo added more data validation added more data validation checks and fixed some typos Update names.py Added check for value errors in to_packed Added check for value errors in extended provisional designation code block of to_packed() missing quotes in example results added missing quotes in example results fixed another bug in to_packed() fixed another bug in to_packed() where designations with 2-digit subscripts were getting skipped fixed error where some packed desigs were failing return statement was missing in to_packed() for provisional designations with no subscript number or subscripts of 1 various changes - added examples of extended provisional designation handling to names.rst - fixed bug in to_packed() - added tests to test_names.py forgot math import command forgot math import command Added/removed white spaces Added/removed white spaces Added extended packed desig functionality Added functionality for extended packed provisional designations to be implemented by the MPC in anticipation of higher discovery rates from LSST (https://minorplanetcenter.net/mpcops/documentation/provisional-designation-definition/)
037c6de
to
e8f5699
Compare
Congrats! |
Added functionality for extended packed provisional designations to be implemented by the MPC in anticipation of higher discovery rates from LSST (https://minorplanetcenter.net/mpcops/documentation/provisional-designation-definition/)