Pull common parts of skymatch from romancal and jwst into stcal #310

WilliamJamieson · 2024-10-17T17:40:56Z

This PR pulls the common parts of skymatch from romancal and jwst into stcal to be shared.

Note, no tests are included because it is well tested by the step tests in both romancal and jwst.

Tasks

update or add relevant tests
update relevant docstrings and / or docs/ page
Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
- write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
- run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")
  - jwst regression test
  - romancal regression test

news fragment change types...

changes/<PR#>.apichange.rst: change to public API
changes/<PR#>.bugfix.rst: fixes an issue
changes/<PR#>.general.rst: infrastructure or miscellaneous change

codecov · 2024-10-17T17:46:42Z

Codecov Report

Attention: Patch coverage is 69.57774% with 317 lines in your changes missing coverage. Please review.

Project coverage is 85.05%. Comparing base (35726e9) to head (6202d38).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/stcal/skymatch/skyimage.py	61.09%	270 Missing ⚠️
src/stcal/skymatch/skymatch.py	83.90%	47 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #310       +/-   ##
===========================================
+ Coverage   29.57%   85.05%   +55.47%     
===========================================
  Files          36       53       +17     
  Lines        7949    10028     +2079     
===========================================
+ Hits         2351     8529     +6178     
+ Misses       5598     1499     -4099

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

emolter

This looks great, thanks William! I just had a few questions about whether some of the ruff and mypy problems could be fixed instead of ignored.

I also had a few questions about the code itself, but those also exist on jwst main, so they're technically beyond the scope of this PR. But I'm still curious to know the answers to those and I figure now is as good a time as any to ensure the code is written as cleanly as possible

emolter · 2025-01-10T14:29:48Z

src/stcal/skymatch/skyimage.py

+        pix_area=1.0,
+        convf=1.0,
+        mask=None,
+        id=None,  # noqa: A002


instead of hiding the style complaint could this be renamed?

I suggest sky_id as it would work with both SkyImage and SkyGroup.

emolter · 2025-01-10T14:31:01Z

src/stcal/skymatch/skyimage.py

+              The functionality to support this conversion is not yet
+              implemented and at this moment `convf` is ignored.


Are there plans to implement this? If not, perhaps now is a good time to remove the option

Based on ancient (>9 years old) discussions with relevant people, this feature, in theory, may not be needed for JWST. In practice, however, we don't know. I suggest leaving this alone as is.

emolter · 2025-01-10T14:40:37Z

src/stcal/skymatch/skyimage.py

+    @property
+    def id(self):  # noqa: A003


I know you most likely just copied all of this from JWST, so not strictly in the scope of this PR, but is there a reason that getter-setter methods are being used here for id, pix_area, sky, is_sky_valid, radec, polygon, and skystat? These don't seem to add any functionality.

These do make it so that this class has the same underscore-protected variable names as SkyGroup, but is there any reason that has to be the case?

Since the original code was written ~11-12 years ago (not sure about this but I think so), I no longer remember the intention. Maybe during the development I thought that, for example, pix_area will be computed on demand but ended up storing it for efficiency. It just allows more flexibility. Also, since both SkyImage and SkyGroup can be used as inputs to match, they should have a common set of properties. I do not remember what was the Python version used during the development but it may have been 2.x (due to PyRAF / Tables/ whatever limitations) so I don't think ABC was available at that time and even if it was, I am not sure properties or attributes of classes could be made abstract. Another reason may have been the desire to get these properties documented (in the docstrings).

I do not think anything should be changed in this PR but in the future they could subclass from a BaseSky, ABCSky or something like that.

I do not see what is the harm in using properties.

no harm it's just unnecessarily verbose since neither the getter nor the setter does anything. It's therefore confusing, because when I see those methods, I expect them to do something. But it doesn't matter- I agree with William's comment that these changes shouldn't go into this PR, if they happen at all.

emolter · 2025-01-10T14:47:36Z

src/stcal/skymatch/skyimage.py

+
+    """
+
+    def __init__(self, images, id=None, sky=0.0):  # noqa: A002


again here can id be renamed instead?

I suggest sky_id as it would work with both SkyImage and SkyGroup.

emolter · 2025-01-10T14:49:42Z

src/stcal/skymatch/skyimage.py

+        for im in self._images:
+            im.sky += sky
+
+    @property


same question here RE why id, polygon, and radec go through the trouble to use @property

emolter · 2025-01-10T14:56:59Z

src/stcal/skymatch/skymatch.py

+log.setLevel(logging.DEBUG)
+
+
+def match(images, skymethod="global+match", match_down=True, subtract=False):


this name also violates https://docs.astral.sh/ruff/rules/builtin-variable-shadowing/ and will likely be changed in JWST by spacetelescope/jwst#9053 to be skymatch()

I would suggest something along the lines do_match or perform_match, or match_sky. The intent for the name of this function was for it to be a verb. skymatch does not sound to me (a non-native English speaker) as a verb but I may be wrong.

I'm happy with whatever name

emolter · 2025-01-10T15:00:37Z

src/stcal/skymatch/skystatistics.py

+from copy import deepcopy
+
+# THIRD PARTY
+from stsci.imagestats import ImageStats  # type: ignore[import-untyped]


might be worth considering making the relevant third-party type ignores global by putting them here

stcal/pyproject.toml

Line 212 in d6a87c3

[[tool.mypy.overrides]]

emolter · 2025-01-10T15:01:41Z

src/stcal/skymatch/skystatistics.py

+    This is a superclass build on top of
+    :py:class:`stsci.imagestats.ImageStats`. Compared to


This comment is confusing because the class doesn't actually inherit from ImageStats

Docs could be changed to explain that SkyStats delegates calls to an ImageStats object if that is more clear but the comment about persistence of settings is important as it is what differentiates it from ImageStats and I think it helps with performance.

I agree that the comment itself explains an important distinction, and yes the simple wording change from "superclass built on top of" to "delegates calls to" would be clearer

WilliamJamieson · 2025-01-10T15:40:39Z

This looks great, thanks William! I just had a few questions about whether some of the ruff and mypy problems could be fixed instead of ignored.

I also had a few questions about the code itself, but those also exist on jwst main, so they're technically beyond the scope of this PR. But I'm still curious to know the answers to those and I figure now is as good a time as any to ensure the code is written as cleanly as possible

I agree they should be fixed. However, as you said they are beyond the scope of this PR. I think for making it possible to trace the history of changes it makes sense to move the code in as close a state as possible from JWST to stcal and then do a follow on PR making cleanups and addressing the issues uncovered here.

kmacdonald-stsci · 2025-01-13T16:23:54Z

src/stcal/skymatch/skyimage.py

+
+        # "bottom" points:
+        borderx[:nptx] = xs
+        bordery[:nptx] = -0.5


I suggest a comment explaining the -0.5 number or the use of a descriptive constant detailing what this number is.

I agree, and I believe this has something to do with all the pixel offsetting. However this is largely just a copy of the stuff from JWST with minimal changes. If someone knows exactly what this is I would be happy to add a comment here.

Really, we should be maintaining a common "constants" object/module to document these and back reference that as opposed to having a bunch of ad hoc magic numbers some of which are documented with comments. This however, is something to consider later.

Centers of pixels have integer image coordinates. Edges of the pixels are located +/- 0.5 pixels away from centers. I think this convention is explained somewhere in the docs. IMO, it does not make sense to add a comment like this every single time we see a 0.5 in the code.

and -0.5 is shorter than coordinate_of_left_pixel_border(_relative_to_center)

I agree with Mihai that in the context of pixels it's relatively clear what a 0.5 offset is doing.
However it looks to me like calc_bounding_polygon is the only function where this is really relevant, so would it work to add a note to the docstring saying something like,

Notes ----- The bounding polygon is defined from corners of pixels whereas the reference pixel is defined at the center of a pixel and therefore the lower-left corner is located at (-0.5, -0.5)

or whatever you want, I'm sure I made some small inaccuracies in describing that but you get the idea

emolter · 2025-01-13T18:23:15Z

I agree they should be fixed. However, as you said they are beyond the scope of this PR.

IMO making the code adhere to the more stringent style rules in stcal remains within the scope. I don't much like ignoring them. I think the downstream testing should assuage any concerns that those changes might break something.

I agree that my comment RE the potentially unnecessary use of @property can be ignored for now though.

WilliamJamieson requested a review from a team as a code owner October 17, 2024 17:40

This was referenced Oct 17, 2024

Switch skymatch to use stcal spacetelescope/romancal#1465

Draft

Use skymatch from stcal spacetelescope/jwst#8901

Draft

nden requested a review from mcara October 22, 2024 17:25

github-actions bot added ramp_fitting testing jump installation labels Dec 19, 2024

WilliamJamieson force-pushed the feature/move_skymatch branch from c7e296c to 9ab3734 Compare December 19, 2024 19:21

github-actions bot added installation and removed ramp_fitting testing jump installation labels Dec 19, 2024

WilliamJamieson force-pushed the feature/move_skymatch branch 2 times, most recently from 9ed58df to cfaf7eb Compare December 20, 2024 14:51

WilliamJamieson force-pushed the feature/move_skymatch branch 2 times, most recently from ba12908 to 2aee80f Compare January 3, 2025 16:16

WilliamJamieson added 5 commits January 3, 2025 12:57

Copy over skymatch from romancal and combine with jwst version

c89afe2

Update changes

2a84121

Update pyproject.toml

c36de6e

Fix check types

b4a3745

Move to using gwcs for region

6202d38

WilliamJamieson force-pushed the feature/move_skymatch branch from 2aee80f to 6202d38 Compare January 3, 2025 17:57

emolter reviewed Jan 10, 2025

View reviewed changes

kmacdonald-stsci approved these changes Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull common parts of skymatch from romancal and jwst into stcal #310

Pull common parts of skymatch from romancal and jwst into stcal #310

WilliamJamieson commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

emolter left a comment

emolter Jan 10, 2025

mcara Jan 13, 2025

emolter Jan 10, 2025

mcara Jan 13, 2025

emolter Jan 10, 2025

mcara Jan 13, 2025

emolter Jan 14, 2025

emolter Jan 10, 2025

mcara Jan 13, 2025

emolter Jan 10, 2025

emolter Jan 10, 2025

mcara Jan 13, 2025

emolter Jan 14, 2025

emolter Jan 10, 2025

emolter Jan 10, 2025

mcara Jan 13, 2025

emolter Jan 14, 2025

WilliamJamieson commented Jan 10, 2025

kmacdonald-stsci Jan 13, 2025

WilliamJamieson Jan 13, 2025

mcara Jan 13, 2025

mcara Jan 13, 2025

emolter Jan 14, 2025 •

edited

Loading

emolter commented Jan 13, 2025

		The functionality to support this conversion is not yet
		implemented and at this moment `convf` is ignored.


		"""

		def __init__(self, images, id=None, sky=0.0): # noqa: A002

		log.setLevel(logging.DEBUG)


		def match(images, skymethod="global+match", match_down=True, subtract=False):

		This is a superclass build on top of
		:py:class:`stsci.imagestats.ImageStats`. Compared to

Pull common parts of skymatch from romancal and jwst into stcal #310

Are you sure you want to change the base?

Pull common parts of skymatch from romancal and jwst into stcal #310

Conversation

WilliamJamieson commented Oct 17, 2024 • edited Loading

Tasks

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

emolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamJamieson commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emolter Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

emolter commented Jan 13, 2025

WilliamJamieson commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

emolter Jan 14, 2025 •

edited

Loading