-
-
Notifications
You must be signed in to change notification settings - Fork 54
Update vizier example #710
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
base: main
Are you sure you want to change the base?
Conversation
lmichel
commented
Nov 25, 2025
- Fix a bug in the processing the space frame equinox.
- Update of the Hipparcos catalogue (I/311/hip2) used as an example in viewer.rst
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
+ Coverage 84.03% 84.04% +0.01%
==========================================
Files 79 79
Lines 8529 8537 +8
==========================================
+ Hits 7167 7175 +8
Misses 1362 1362 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looks good to me. I'd remove the mention of the catalog change from the changelog: it's not really a bug, it's just recommended to use to recalculated version. And it's only in the documentation so it's ok if this is not mentioned in the changelog. |
bsipocz
left a comment
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.
Minor comments, none of which is really relevant to this PR.
| - Fix a bug in the space frame equinox processing. | ||
| Update of the Hipparcos catalogue (I/311/hip2) used in the viewer doc [710] |
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.
No need for the docs mentions in the changelog, and fix PR number syntax
| - Fix a bug in the space frame equinox processing. | |
| Update of the Hipparcos catalogue (I/311/hip2) used in the viewer doc [710] | |
| - Fix a bug in the space frame equinox processing. [#710] |
| raise MappingError(f"Cannot interpret field {hk_field} " | ||
| f"as a {('besselian' if besselian else 'julian')} timestamp") | ||
|
|
||
| time_instance = self. _build_time_instance(timestamp, representation, besselian) |
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.
With this typo, I suspect this line doesn't get test coverage
| time_instance = self. _build_time_instance(timestamp, representation, besselian) | |
| time_instance = self._build_time_instance(timestamp, representation, besselian) |
| Check the SkyCoord instance against the constant values given as parameters | ||
| """ | ||
| try: | ||
| assert scoo.ra.degree == pytest.approx(ra) |
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.
I haven't noticed this before, but in general we don't really use pytest.approx() but prefer the functionality from numpy.testing. It's not really a big issue here when comparing scalars, but elsewhere the numpy versions may be more preferred.
| } | ||
|
|
||
|
|
||
| def check_skycoo(scoo, ra, dec, distance, pm_ra_cosdec, pm_dec, obstime): |
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.
I wonder if there is a better way to compare the values to a SkyCoord object.