Skip to content

ENH: improving error messaging for invalid SIAv2 inputs #693

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

Merged
merged 3 commits into from
Aug 17, 2025

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Jul 30, 2025

This keeps bugging me, so while we think about possible workarounds in #692, I would just make the error message more descriptive than the current one propagated from astropy:

TypeError: Scalar 'SkyCoord' object has no len()

Technically this is an API change as we change the error type from TypeError to ValueError but I feel this is still backportable improvement (no user code should have try/excepted on this TypeError).

@bsipocz bsipocz force-pushed the ENH_clearer_error_for_pos branch from 8bae10a to 6469682 Compare July 30, 2025 19:50
Copy link

codecov bot commented Jul 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.02%. Comparing base (30a4ecb) to head (6469682).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
+ Coverage   84.00%   84.02%   +0.01%     
==========================================
  Files          80       80              
  Lines        8522     8524       +2     
==========================================
+ Hits         7159     7162       +3     
+ Misses       1363     1362       -1     

☔ 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.

Copy link
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

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

I trust this is a UI improvement and approve of it as such. However, even a quick glance at the SkyCoord docs in astropy did not reveal the logic of pos.size<4 to me (it doesn't help that my version of the astropy docs doesn't even mention the size attribute). I also couldn't locate the place where an appropriately-sized SkyCoord would turn into a polygon (if that's what it is). So: I'd say a comment is in order here. But perhaps I'm just being particularly dense here.

While you're editing this: Could you add the missing "been" in _validate_pos' docstring?

Feel free to merge, though.

@bsipocz
Copy link
Member Author

bsipocz commented Aug 17, 2025

Yes, the size is inherited property and those seem to be not exposed to the API docs, I found it with TAB completion (and now its only mention in the docs is here: https://docs.astropy.org/en/stable/coordinates/skycoord.html#attributes).

@bsipocz
Copy link
Member Author

bsipocz commented Aug 17, 2025

Feel free to merge, though.

I'll go ahead with this, with the known caveat that this may only land in 1.7.1 as I plan to follow-up on our discussion in #692 and add a default small radius for v1.8

@bsipocz bsipocz merged commit 3fe6e80 into astropy:main Aug 17, 2025
8 of 11 checks passed
@bsipocz bsipocz deleted the ENH_clearer_error_for_pos branch August 17, 2025 05:37
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.

2 participants