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

DM-38575: Change filterData to floats and add seed #766

Merged
merged 1 commit into from Apr 12, 2023

Conversation

cmsaunders
Copy link
Contributor

No description provided.

@erykoff
Copy link
Contributor

erykoff commented Apr 6, 2023

Two things before a full review ... first, I don't see the seed change, was that not committed? Second, the whole discussion at scikit-image/scikit-image#6871 makes me really, really, really, really, really worried that we are relying on this code. Because it does not look reliable at all.

@stefanv
Copy link

stefanv commented Apr 7, 2023

Two things before a full review ... first, I don't see the seed change, was that not committed? Second, the whole discussion at scikit-image/scikit-image#6871 makes me really, really, really, really, really worried that we are relying on this code. Because it does not look reliable at all.

@erykoff I understand your concern (although I may only have used one or two really's 😉); I would feel the same if I were building a pipeline that needed to produce highly reliable and consistent results over a very long period of time. There are some obvious ways to address that:

  1. Pin the skimage version
  2. Vendor the code
  3. Use an alternative implementation (e.g., OpenCV2)
  4. Implement a canny edge detector yourselves

From the skimage perspective, let me address the principle violated:

We strive to never have code changes introduce changes in numerical results, unless it fixes a bug that gets us closer to the "true" answer. This is the reason behind the skimage v2 redesign, so we have the freedom to make some such changes.

The change you witnessed was introduced inadvertently. We consider this of a breach of contract with our users, and for that I apologize. When I saw your bug report, I spent most of the day debugging it; my primary goal here is to ensure that whatever results you see, they are correct.

EDIT: The change turned out to be a fix for border conditions.

I cannot speak as to the wisdom of relying on skimage; but I want to clarify the intent of the team working on it. Also thank you for flagging this bug; while we do our best to test our own code, we are very grateful for extensive and careful testing done by the community.

@cmsaunders cmsaunders merged commit a751136 into main Apr 12, 2023
1 check passed
@cmsaunders cmsaunders deleted the tickets/DM-38575 branch April 12, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants