Skip to content
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

replace opencv-python with scikit-image for snowball detection #138

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zacharyburnett
Copy link
Collaborator

@zacharyburnett zacharyburnett commented Dec 29, 2022

Closes spacetelescope/jwst#7409

This PR adds fallback functions for replaces ellipse construction using scikit-image and shapely, removing opencv-python entirely. The scikit-image + shapely methods produce slightly different contours, ellipses, and circles than opencv-python.

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Dec 29, 2022

Contour-finding also needs to be ported from opencv-python to scikit-image:

ModuleNotFoundError: `opencv-python` required for this functionality
def find_circles(dqplane, bitmask, min_area):
        # Using an input DQ plane this routine will find the groups of pixels with at least the minimum
        # area and return a list of the minimum enclosing circle parameters.
        pixels = np.bitwise_and(dqplane, bitmask)
        if ELLIPSE_PACKAGE == 'opencv-python':
            contours, hierarchy = cv.findContours(pixels, cv.RETR_EXTERNAL, cv.CHAIN_APPROX_SIMPLE)
            bigcontours = [con for con in contours if cv.contourArea(con) >= min_area]
            circles = [cv.minEnclosingCircle(con) for con in bigcontours]
        else:
            # raise ModuleNotFoundError(ELLIPSE_PACKAGE_WARNING)
           raise ModuleNotFoundError('`opencv-python` required for this functionality')

@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Attention: Patch coverage is 94.16667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 85.35%. Comparing base (3b924fb) to head (cf527d6).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/jump/circle.py 87.23% 12 Missing ⚠️
tests/test_circle.py 96.72% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   84.74%   85.35%   +0.61%     
==========================================
  Files          44       46       +2     
  Lines        8527     8693     +166     
==========================================
+ Hits         7226     7420     +194     
+ Misses       1301     1273      -28     

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

@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Dec 29, 2022

As described in #126 (comment), there are some differences in the methods used to construct ellipses between the two packages which result in different image masks. Perhaps a warning should make this more clear.

looking into replacing shape construction with scikit-image, it appears there is a crucial difference in the drawing method:

import cv2
import numpy
from matplotlib import pyplot
from skimage import draw

image_0 = numpy.full((10, 10), fill_value=0, dtype=numpy.uint8)

center = (4, 4)
radius = 4

disk = draw.disk(center, radius)
image_1 = image_0.copy()
image_1[disk] = 1

image_2 = cv2.circle(image_0.copy(), center, radius, 1, thickness=-1)

figure, axes = pyplot.subplots(nrows=1, ncols=2)

axes[0].imshow(image_1)
axes[0].set_title('scikit-image')

axes[1].imshow(image_2)
axes[1].set_title('opencv')

pyplot.show()

Figure_1

@zacharyburnett zacharyburnett marked this pull request as ready for review December 30, 2022 16:06
@zacharyburnett zacharyburnett marked this pull request as draft December 30, 2022 16:32
@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Dec 30, 2022

It looks like the contours found by scikit-image are consistently larger, by about half a unit, than those found by opencv:

>       assert circle[0][1] == pytest.approx(1.0, 1e-3)
E       assert 1.5 == 1.0 ± 1.0e-03

comparison

I assume this is a difference in methodology in thinking about pixels as blocks vs as points.

@zacharyburnett
Copy link
Collaborator Author

expanding the ellipse test case to a more oblong shape reveals a few edge cases in the scikit-image method; I'll convert this to draft for now and work on that

@zacharyburnett zacharyburnett marked this pull request as draft December 30, 2022 18:22
@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Dec 30, 2022

added some more complexity to the test shape and fixing issues in the ellipse fitting:
comparison

@@ -40,23 +45,25 @@ def test_find_simple_circle():
plane[2, 3] = DQFLAGS['JUMP_DET']
plane[2, 1] = DQFLAGS['JUMP_DET']
circle = find_circles(plane, DQFLAGS['JUMP_DET'], 1)
assert circle[0][0] == pytest.approx((2, 2))
assert circle[0][1] == pytest.approx(1.0, 1e-3)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here is where the fit circle from the scikit-image contour differs from the opencv-python circle; the radius is 1.5 instead of 1

@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Jan 3, 2023

Currently, the .minAreaRect() is replaced by .minimum_rotated_rectangle from Shapely; this algorithm could be written without Shapely, like in circle.py, to remove the need for the additional dependency

src/stcal/jump/circle.py Outdated Show resolved Hide resolved
src/stcal/jump/circle.py Show resolved Hide resolved
src/stcal/jump/circle.py Outdated Show resolved Hide resolved
src/stcal/jump/circle.py Outdated Show resolved Hide resolved
src/stcal/jump/circle.py Outdated Show resolved Hide resolved
src/stcal/jump/circle.py Show resolved Hide resolved
@zacharyburnett
Copy link
Collaborator Author

zacharyburnett commented Jan 4, 2023

we should also get input from INS on whether this alternative is acceptable

@zacharyburnett
Copy link
Collaborator Author

started regression test run against this branch at https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST/detail/JWST/2925/pipeline

@drlaw1558
Copy link
Contributor

Hm, I'm testing this on jw02123001001_09101_00001_nrs1 and something doesn't seem to be working right. I started out by merging the latest main branch in to make sure everything was synced, and then tried processing jw02123001001_09101_00001_nrs1

Runtime was significantly longer for the Scikit version; 743 seconds vs 314 seconds for the current opencv

More problematically though, the Scikit version doesn't seem to be flagging snowballs properly. It's flagging lots of little things that don't need attention, but missing the big snowballs. Perhaps I'm running the code the wrong way? In both cases I'm just using strun to execute detector1 pipeline, with the default expand_large_events=True

scikit

@zacharyburnett
Copy link
Collaborator Author

Thanks for testing! Where is jw02123001001_09101_00001_nrs1? I can run detector1 and debug that; it should have similar results to opencv, so if it doesn't then something is wrong

@zacharyburnett
Copy link
Collaborator Author

oh I can search that on MAST! Nevermind I'll use that, thanks anyway! 😅

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.

Opencv dependency in stcal being optional causes uncaught error in jump step
6 participants