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

Make unwrap_phase fail for 2D input containing NaNs #7176

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lagru
Copy link
Member

@lagru lagru commented Sep 29, 2023

Description

Closes #3831.

After some stone-age debugging with print statements, it seems to me that the infinite loop happens inside

EDGE *partition(EDGE *left, EDGE *right, double pivot) {
while (left <= right) {
while (left->reliab < pivot) ++left;
while (right->reliab >= pivot) --right;
if (left < right) {
swap(*left, *right);
++left;
--right;
}
}
return left;
}

which never exits due to pivot being NaN. Current solution is to check pivot before passing it to that function and return error codes up the call stack until we can raise a proper Python exception.

Checklist

Release note

Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically:

Raise a `RuntimeError` in `skimage.restoration.unwrap_phase` If a NaN is encountered 
while sorting edges. This may happen if the input `image` contains unmasked NaNs.

It seems that the infinite loop happens inside the function `partition`
which never exits due to `pivot` being NaN. So check `pivot` before
passing it to that function and return error codes up the call stack
until we can raise a proper Python exception.

Not sure if that is the best way to address the problem. I understand
little of what's going on, but it's better than hanging with one core
stuck at 100%. Performance wise I didn't see any significant impact for
the following snippet taken from plot_phase_unwrap.py:

```python
import numpy as np
from skimage import data, img_as_float, color, exposure
from skimage.restoration import unwrap_phase
image = color.rgb2gray(img_as_float(data.chelsea()))
image = exposure.rescale_intensity(image, out_range=(0, 4 * np.pi))
image_wrapped = np.angle(np.exp(1j * image))

%timeit unwrap_phase(image_wrapped)
```
@lagru lagru added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Sep 29, 2023
Comment on lines +148 to +150
if (isnan(pivot)) {
return NAN_ERROR;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if that is the best way to address the problem. I understand little of what's going on, but it's better than hanging with one core stuck at 100%. Performance wise I didn't see any significant impact for the following snippet taken from plot_phase_unwrap.py:

import numpy as np
from skimage import data, img_as_float, color, exposure
from skimage.restoration import unwrap_phase
image = color.rgb2gray(img_as_float(data.chelsea()))
image = exposure.rescale_intensity(image, out_range=(0, 4 * np.pi))
image_wrapped = np.angle(np.exp(1j * image))

%timeit unwrap_phase(image_wrapped)

Though I am a bit dubious because the paper makes it seem like there should be a huge amount of edges and I am not sure how often quicker_sort is called in the total scheme of things.

@lagru
Copy link
Member Author

lagru commented Sep 29, 2023

Ugh, this snippet (from #3831 (comment)) still fails

import numpy as np
from skimage.restoration import unwrap_phase
wrapped = np.empty((100, 100), dtype=np.float64)
wrapped[:] = np.linspace(0, 4*np.pi, 100) % (2*np.pi)
wrapped[10, 10] = np.nan  #  <-- comment out to get working code
masked = np.ma.masked_array(wrapped, mask=np.isnan(wrapped))
print(np.isnan(masked).any())
unwrapped = unwrap_phase(masked)

Copy link

Hello scikit-image core devs! There hasn't been any activity on this PR for more than 180 days. I have marked it as "dormant" to make it easy to find.
To our contributors, thank you for your contribution and apologies if this contribution fell through the cracks! Hopefully this ping will help bring some fresh attention to the review. If you need help, you can always reach out on our forum
If you think that this PR is no longer relevant, you may close it, or we may do it at some point (either way, it will be done manually). If you think the PR is valuable but you no longer have the bandwidth to update it, please let us know, so that someone can take it over. 🙏

@github-actions github-actions bot added 😴 Dormant no recent activity and removed 😴 Dormant no recent activity labels Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Freeze while passing NaN to unwrap_phase
1 participant