Enable phase_center inputs to be SkyCoord#77
Open
DanRyanIrish wants to merge 13 commits intoTCDSolar:mainfrom
Open
Enable phase_center inputs to be SkyCoord#77DanRyanIrish wants to merge 13 commits intoTCDSolar:mainfrom
DanRyanIrish wants to merge 13 commits intoTCDSolar:mainfrom
Conversation
Good for representing images from unknown instruments.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
=======================================
Coverage ? 15.28%
=======================================
Files ? 8
Lines ? 968
Branches ? 0
=======================================
Hits ? 148
Misses ? 820
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Contributor
Author
|
Do not merge until #76 is merged. |
There was a problem hiding this comment.
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- changelog/76.feature.rst: Language not supported
- changelog/77.feature.rst: Language not supported
Comments suppressed due to low confidence (2)
xrayvision/visibility.py:484
- Directly comparing SkyCoord objects with '!=' may lead to unexpected results due to floating point precision. Consider using SkyCoord's is_equivalent() method or a comparison based on separation.
if self.phase_center != other.phase_center:
xrayvision/transform.py:164
- [nitpick] The custom attributes 'Tx' and 'Ty' on the SkyCoord object are non-standard and could be confusing. Consider clarifying their purpose or using more conventional attribute names in documentation.
y = generate_xy(m, phase_center.Ty, pixel_size=pixel_size[0]) # type: ignore
Comment on lines
+34
to
35
| stix_vis.phase_center = SkyCoord(Tx=stix_vis.phase_center[1], Ty=stix_vis.phase_center[0], frame=Projective) | ||
|
|
There was a problem hiding this comment.
Indexing a SkyCoord object as if it were a sequence may result in an error since phase_center is expected to be a scalar SkyCoord. Use the existing attributes (e.g. phase_center.Tx and phase_center.Ty) instead of indexing.
Suggested change
| stix_vis.phase_center = SkyCoord(Tx=stix_vis.phase_center[1], Ty=stix_vis.phase_center[0], frame=Projective) | |
| stix_vis.phase_center = SkyCoord(Tx=stix_vis.phase_center.Tx, Ty=stix_vis.phase_center.Ty, frame=Projective) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR enables the
phase_centerinputs to theVisibilitiescalculating functions to be given as aSkyCoord. The result is that theVisibilitiesobject knows its phase center in a valid coordinate frame, and hence enable imaging routines to returnMaps in the same coordinate frame.