Skip to content

Conversation

@hhsieh00
Copy link
Collaborator

Fixing a bug where parse_comet() incorrectly interprets input like "2015XN77" as a well-formed comet designation (where number=2015, type=X, and name=N77)

Added a line to disallow comet names that have a number in the second position in order to avoid having parse_comet() incorrectly interpret input like "2015XN77" as a well-formed comet designation (where number=2015, type=X, and name=N77)
@hhsieh00 hhsieh00 linked an issue Feb 12, 2025 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Feb 12, 2025

Thank you for your contribution to sbpy, an Astropy affiliated package! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the sbpy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related?
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label.
  • Is a milestone added?

@codecov
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.63%. Comparing base (9360983) to head (dcc38dc).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #422      +/-   ##
==========================================
+ Coverage   84.61%   84.63%   +0.01%     
==========================================
  Files          92       92              
  Lines        8420     8426       +6     
==========================================
+ Hits         7125     7131       +6     
  Misses       1295     1295              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

added test invalid input
@hhsieh00
Copy link
Collaborator Author

So the problem in issue #421 seems to actually be due to the second part of the asteroid designation also being a potential comet type, so parse_comet will interpret "2015XN77", "2015PN77", and "2015CN77" all as comets (broken down as number=2015, type=X/P/C, and name=N77), while "2015TN77" or any other letters in that first spot besides PDCX will (correctly) throw an error. The simplest fix seems to be to apply a validation test to the comet name to disallow comet names with numbers in the second position (but allow other non-alphabetic characters so names like d'Arrest don't also get screened out). I considered adding a validation test earlier on in the parsing to look for years followed by asteroid designations without spaces, but I feel like that has a better chance of introducing unintended consequences (especially once LSST takes us into the four-digit comet number era), whereas simply validating the name seems to address this specific issue without impacting any other functionality.

@hhsieh00 hhsieh00 marked this pull request as draft February 12, 2025 04:05
…as packed asteroid desigs

Fixed additional bug where a provisional comet designation with no forward slash and no spaces (e.g., "P2015XN77") was being interpreted as a packed asteroid designation (using only the "P2015" portion of the designation, which is technically unpackable into "252015"), but now raises a TargetNameParseError with the addition of a check to make sure a detected packed designation is not just a portion of a longer input string.
@hhsieh00 hhsieh00 marked this pull request as ready for review February 12, 2025 10:22
@hhsieh00 hhsieh00 requested a review from mkelley February 12, 2025 10:22
@mkelley
Copy link
Member

mkelley commented Feb 12, 2025

Thanks!

@mkelley mkelley added this to the v0.6 milestone Feb 12, 2025
@hhsieh00 hhsieh00 merged commit f716f24 into main Feb 12, 2025
10 checks passed
@hhsieh00 hhsieh00 deleted the 421-incorrect-classification-of-namesasteroid_or_comet branch February 12, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect Classification of Names.asteroid_or_comet

3 participants