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

Freeze while passing NaN to unwrap_phase #3831

Open
ldes89150 opened this issue Apr 10, 2019 · 13 comments · May be fixed by #7176
Open

Freeze while passing NaN to unwrap_phase #3831

ldes89150 opened this issue Apr 10, 2019 · 13 comments · May be fixed by #7176
Labels

Comments

@ldes89150
Copy link

Description

I found in some situation the program will freeze after passing the array contains NaN values. Here is the minimum example I found that can reproduce the error. I'd like to know if there is a way to deal with missing data.
Thanks!

Way to reproduce

import numpy as np                                                      
from skimage.restoration import unwrap_phase   

x = np.linspace(1,100)   
x[1] = np.nan    
xx ,yy = np.meshgrid(x,x)   
unwrap_phase(xx)

Version information

3.7.1 (default, Dec 14 2018, 19:28:38) 
[GCC 7.3.0]
Linux-4.9.0-8-amd64-x86_64-with-debian-9.8
scikit-image version: 0.14.2
numpy version: 1.15.4
@jni
Copy link
Member

jni commented Apr 11, 2019

@ldes89150 thanks for the report! The code should never freeze, that's definitely a bug. However, to indicate masked values, you should use a NumPy masked array. The second part of the gallery example shows how to use this:

http://scikit-image.org/docs/dev/auto_examples/filters/plot_phase_unwrap.html

@scikit-image/core longer term, perhaps we should add a mask argument to the function that has the same semantics as the rest of the library?

@stefanv
Copy link
Member

stefanv commented Apr 11, 2019

@ldes89150 bug confirmed.
@jni agreed.

@hmaarrfk
Copy link
Member

Quickly looking at the code, it seems there are some floating point comparisons that aren't NaN safe in the C code.

Not sure the best way to deal with this that won't completely kill performance.

@stefanv
Copy link
Member

stefanv commented Apr 11, 2019 via email

@hmaarrfk
Copy link
Member

hmaarrfk commented Apr 12, 2019

Agreed. I think it is the floating point comparison that is tripping off the algorithm

https://github.com/scikit-image/scikit-image/blob/master/skimage/restoration/unwrap_2d_ljmu.c#L100

@jni
Copy link
Member

jni commented Apr 12, 2019

typedef enum {
  yes,
  no
} yes_no;

yes_no find_pivot(EDGE *left, EDGE *right, double *pivot_ptr) {

Really, bools were not sufficiently expressive here? 🤦‍♂️

@hmaarrfk a hang suggests a while loop is involved rather than a for-loop. I notice two whiles later on in that function...

@stefanv perhaps np.nan_to_num would be useful here? =)

@hmaarrfk
Copy link
Member

We should look at how other NaN safe functions are implemented

@ldes89150
Copy link
Author

@jni Thanks for the suggestion of using masked arrays!

@plu2k
Copy link

plu2k commented May 27, 2019

Masking arrays unfortunately does not work for me. I have a 3000 x 3000 float array with NaNs masked out using MaskedArrays. Testing numpy.isnan(arr).any() returns False.
Maybe it is worth mentioning that the NaNs cover about 25% of the "outer" regions of the array including complete rows / columns. The rest is well connected, though. I did check value ranges.

@jni
Copy link
Member

jni commented May 27, 2019

@plu2k are you able to share your masked array with us for testing?

@plu2k
Copy link

plu2k commented May 28, 2019

This is strange. Although using masked arrays works in some cases, when trying to corner the problem I found this minimal example:

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)

which causes a freeze (skimage 0.15.0, numpy 1.16.2).

@sciunto sciunto added this to the 0.17 milestone Aug 9, 2019
@grlee77
Copy link
Contributor

grlee77 commented Feb 28, 2022

This is strange. Although using masked arrays works in some cases, when trying to corner the problem I found this minimal

Thanks, @plu2k, I can confirm that this is still the case on v0.19.2

@jarrodmillman jarrodmillman modified the milestones: 0.17, 0.20 Jun 4, 2022
@grlee77 grlee77 modified the milestones: 0.20, 0.21 Dec 4, 2022
@jarrodmillman jarrodmillman modified the milestones: 0.21, 0.22 May 19, 2023
@lagru lagru added the 🐛 Bug label Sep 16, 2023
@lagru lagru linked a pull request Sep 29, 2023 that will close this issue
@lagru
Copy link
Member

lagru commented Sep 29, 2023

See #7176 for an attempted fix.

@jarrodmillman jarrodmillman removed this from the 0.22 milestone Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants