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

TST: Verify that output of estimate_sigma is a proper input to nlmeans. #721

Merged
merged 5 commits into from Oct 5, 2015

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Sep 25, 2015

No description provided.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Sep 26, 2015

This seems to work fine! Just a quick question - why did you create a separate file for this test instead of adding it in test_nlmeans?

@arokem

This comment has been minimized.

Member

arokem commented Sep 29, 2015

Could someone please take a look here? Seems to me that i fixes a regression that is currently on masater and is also the only thing currently holding up merging #677

@arokem

This comment has been minimized.

Member

arokem commented Sep 29, 2015

Answering @RafaelNH's question above: I am thinking that we might one day write a nice API that will put all of these things together under the denoise name-space, so I am anticipating that.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Sep 29, 2015

Turns out I did actually patch my script to make it work instead of changing the output of the function, a small mistake. Although, the way it is written here, anyone using a 3D array will see the function failing on him. Would it be possible to write it in a way to always broadcast the nd input to a full 4D array?

@arokem

This comment has been minimized.

Member

arokem commented Sep 29, 2015

What exactly is the failure mode you are referring to? The last commit adds a smoke test for 3D input, and that seems to run with no error. If you could write out the code that you are seeing a failure for, it will help me add this as a test and writing the code to prevent that failure from happening.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Sep 29, 2015

A 3D array for sigma and a 4D array to denoise should not work the way it's written, so one needs to broadcast it to 4D beforehand. The same thing is true for a 2D array, it would be easier for it to be broadcasted in the function itself for convenience.

So for example np.ones((3,3,3)) * np.ones((3,3,3,10)) does not work, but np.ones((3,3,3,1)) * np.ones((3,3,3,10)) does. The suggestion mentioned in the DKI thread would be to do the broadcasting inside the nlmeans function for convenience I guess.

@arokem

This comment has been minimized.

Member

arokem commented Sep 29, 2015

Broadcast what? The sigma input?

What is the code for a test that would fail with this implementation, but
you think should pass?

On Tue, Sep 29, 2015 at 5:02 AM, Samuel St-Jean notifications@github.com
wrote:

A 3D array for sigma and a 4D array to denoise should not work the way
it's written, so one needs to broadcast it to 4D beforehand. The same thing
is true for a 2D array, it would be easier for it to be broadcasted in the
function itself for convenience.

So for example np.ones((3,3,3)) * np.ones((3,3,3,10)) does not work, but
np.ones((3,3,3,1)) * np.ones((3,3,3,10)) does. The suggestion mentioned in
the DKI thread would be to do the broadcasting inside the nlmeans function
for convenience I guess.


Reply to this email directly or view it on GitHub
#721 (comment).

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Sep 29, 2015

data = np.ones((10,10,10,5))
sigma = np.ones((10,10,10))

nlmeans(data, sigma)


ValueError Traceback (most recent call last)
in ()
57 sigma = np.ones((10,10,10))
58
---> 59 nlmeans(data, sigma)

in nlmeans(arr, sigma, mask, patch_radius, block_radius, rician)
38
39 denoised_arr = np.zeros_like(arr)
---> 40 sigma = np.ones(arr.shape, dtype=np.float64) * sigma
41
42 for i in range(arr.shape[-1]):

ValueError: operands could not be broadcast together with shapes (10,10,10,5) (10,10,10)

The idea would be to allow a 3D sigma array to broadcast to a 4D one so this call would be valid. This is what the original did, but it also broke estimate_sigma by mistake, so having both behaviour at the same would probably cover all use cases.

@arokem

This comment has been minimized.

Member

arokem commented Sep 29, 2015

Gotcha.
On Sep 29, 2015 8:37 AM, "Samuel St-Jean" notifications@github.com wrote:

data = np.ones((10,10,10,5))
sigma = np.ones((10,10,10))

nlmeans(data, sigma)

ValueError Traceback (most recent call last)
in ()
57 sigma = np.ones((10,10,10))
58
---> 59 nlmeans(data, sigma)

in nlmeans(arr, sigma, mask, patch_radius, block_radius, rician)
38
39 denoised_arr = np.zeros_like(arr)
---> 40 sigma = np.ones(arr.shape, dtype=np.float64) * sigma
41
42 for i in range(arr.shape[-1]):

ValueError: operands could not be broadcast together with shapes
(10,10,10,5) (10,10,10)

The idea would be to allow a 3D sigma array to broadcast to a 4D one so
this call would be valid. This is what the original did, but it also broke
estimate_sigma by mistake, so having both behaviour at the same would
probably cover all use cases.


Reply to this email directly or view it on GitHub
#721 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Sep 30, 2015

Does this look good to you, @samuelstjean?

@arokem

This comment has been minimized.

Member

arokem commented Oct 3, 2015

Any thoughts here from anyone? It's a really small amount of code to fix a current bug. Maybe someone can take a look.

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Oct 5, 2015

Hi @arokem!

I tried to run our new version of nlmeans in my local computer. I have the following error when trying the following command:

fdata, fbval, fbvec = dpd.get_data()

Test on 4D image:

data = nib.load(fdata).get_data()
sigma1 = estimate_sigma(data)
denoised = nlmeans(data, sigma=sigma1)

Traceback (most recent call last):

File "", line 5, in
denoised = nlmeans(data, sigma=sigma1)

File "", line 50, in nlmeans
rician).astype(arr.dtype)

File "dipy\denoise\denspeed.pyx", line 52, in dipy.denoise.denspeed.nlmeans_3d (dipy\denoise\denspeed.c:2287)
arr = np.ascontiguousarray(arr, dtype='f8')

File "dipy\denoise\denspeed.pyx", line 76, in dipy.denoise.denspeed._nlmeans_3d (dipy\denoise\denspeed.c:2623)
double summ = 0

TypeError: only length-1 arrays can be converted to Python scalars

Checks have passed and the code looks fine. However, I am trying to understand what is happening in my local computer. What could be the problem?

@arokem

This comment has been minimized.

Member

arokem commented Oct 5, 2015

Did you run python setup.py build_ext again since updating? There are
some changes in the cython code.

On Mon, Oct 5, 2015 at 10:37 AM, Rafael Neto Henriques <
notifications@github.com> wrote:

Hi @arokem https://github.com/arokem!

I tried to run our new version of nlmeans in my local computer. I have the
following error when trying the following command:

fdata, fbval, fbvec = dpd.get_data()
Test on 4D image:

data = nib.load(fdata).get_data()
sigma1 = estimate_sigma(data)
denoised = nlmeans(data, sigma=sigma1)

Traceback (most recent call last):

File "", line 5, in
denoised = nlmeans(data, sigma=sigma1)

File "", line 50, in nlmeans
rician).astype(arr.dtype)

File "dipy\denoise\denspeed.pyx", line 52, in
dipy.denoise.denspeed.nlmeans_3d (dipy\denoise\denspeed.c:2287)
arr = np.ascontiguousarray(arr, dtype='f8')

File "dipy\denoise\denspeed.pyx", line 76, in
dipy.denoise.denspeed._nlmeans_3d (dipy\denoise\denspeed.c:2623)
double summ = 0

TypeError: only length-1 arrays can be converted to Python scalars

Checks have passed and the code looks fine. However, I am trying to
understand what is happening in my local computer. What could be the
problem?


Reply to this email directly or view it on GitHub
#721 (comment).

Garyfallidis added a commit that referenced this pull request Oct 5, 2015

Merge pull request #721 from arokem/estimate-sigma-to-nlmeans
TST: Verify that output of estimate_sigma is a proper input to nlmeans.

@Garyfallidis Garyfallidis merged commit 94162ce into nipy:master Oct 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Oct 9, 2015

Hi @arokem!
Sorry for only getting back to you now - the last days I have been travelling for some meeting with some collaborators, so only I had the time to get back to this problem this morning.
FYI - Indeed the problem was on changes of the cython code. They were sorted after I run python setup.py build_ext --inplace (not only python setup.py build_ext). I think this information could be useful if someone contacts you with a similar issue ;).

@arokem

This comment has been minimized.

Member

arokem commented Oct 9, 2015

Good to hear, and good to have here for reference. FYI: I presented your
DKI work to the neuroimaging lab here at UW yesterday. We might soon have
users for these methods :-)

On Fri, Oct 9, 2015 at 3:42 AM, Rafael Neto Henriques <
notifications@github.com> wrote:

Hi @arokem https://github.com/arokem!
Sorry for only getting back to you now - the last days I have been
travelling for some meeting with some collaborators, so only I had the time
to get back to this problem this morning.
FYI - Indeed the problem was on changes of the cython code. They were
sorted after I run python setup.py build_ext --inplace (not only python
setup.py build_ext). I think this information could be useful if someone
contacts you with a similar issue ;).


Reply to this email directly or view it on GitHub
#721 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment