-
Notifications
You must be signed in to change notification settings - Fork 437
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
Updated docs for using P2S optimally #2382
Conversation
ShreyasFadnavis
commented
May 1, 2021
•
edited
Loading
edited
- Added information about parameters of Patch2Self
- Also added information on applying Patch2Self on batches
Hi! Sorry for the slowness in this small PR. I have added information about how to use Patch2Self optimally. It is ready to be reviewed 👍🏽 |
doc/examples/denoise_patch2self.py
Outdated
ridge regression (model='ridge'). | ||
|
||
*Please do note that sometimes using ridge regression can hamper the | ||
performance of Patch2Self. If so, please use model='ols'.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
performance of Patch2Self. If so, please use model='ols'.* | |
performance of Patch2Self. If so, please use model='ols'. |
doc/examples/denoise_patch2self.py
Outdated
After doing this, the 2 denoised batches can be merged as follows: | ||
`denoised_p2s = np.concatenate((denoised_batch1, denoised_batch2), axis=3)` | ||
|
||
If using Patch2Self for research purposes, acquisition design tests, etc., one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If using Patch2Self for research purposes, acquisition design tests, etc., one | |
If using Patch2Self for research purposes, acquisition design tests, etc., one |
I am not sure what the "research purposes" means here. The following "acquisition design, tests, etc." suggests that this is not something that users should do if they want to analyze the data? But I am not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Agreed... Taking that part of the line off. I just wanted to encourage people to experiment with the algorithm.
doc/examples/denoise_patch2self.py
Outdated
denoised_arr = patch2self(data, bvals, shift_intensity=True, | ||
clip_negative_vals=False) | ||
denoised_arr = patch2self(data, bvals, model='ols', shift_intensity=True, | ||
clip_negative_vals=False, b0_threshold=100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry -- one more: why is b0_threshold
set to a higher value than the gradient_table
default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay... Set it back to 50, which is the default (even in the codebase). I added a note that people should change it if they are working with data such as HCP 👍🏽
doc/examples/denoise_patch2self.py
Outdated
|
||
The default value of `b0_threshold` in DIPY is set to 50. If using data | ||
such as HCP, the b0 volume tends to have a higher value, please set it to 100 | ||
or more accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. The b-values that were used in the HCP 3T dataset are:
array([ 5., 990., 995., 1000., 1005., 1985., 1990., 1995., 2000.,
2005., 2010., 2980., 2985., 2990., 2995., 3000., 3005., 3010.])
So, setting b0_threshold=50
and setting b0_threshold=100
would presumably give you exactly the same thing. I am also confused about the phrase the b0 volume tends to have a higher value
-- it doesn't seem like this parameter would interact with the values of the image, only with the selection of the image volumes. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @arokem -- That is correct. If the bvals are less than 50 the b0_threshold
at 50 and 100 would behave exactly the same way. Here are the bvals for HCP 7T for instance:
As you can see they are >=50 somewhere between 55 and 65. Although not common, I do want to warn users in case they have such datasets. Let me try rephrasing the sentence to make it clearer.
doc/examples/denoise_patch2self.py
Outdated
denoised_arr = patch2self(data, bvals, shift_intensity=True, | ||
clip_negative_vals=False) | ||
denoised_arr = patch2self(data, bvals, model='ols', shift_intensity=True, | ||
clip_negative_vals=False, b0_denoising=50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clip_negative_vals=False, b0_denoising=50) | |
clip_negative_vals=False, b0_threshold=50) |
More here: I believe that b0_denoising
is a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or possibly
clip_negative_vals=False, b0_denoising=50) | |
clip_negative_vals=False, b0_denoising=True) |
But both are the default values, so it's not clear why you are setting either of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! Good catch -- fixing this.
|
||
""" | ||
The above parameters should give optimal denoising performance for Patch2Self. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimal in this dataset, or generally? If generally, why are these not the default values? For example, the warning below about ridge hampering performance is rather ominous. Should we change the default to 'ols'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimal in this dataset, or generally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'ols' is optimal generally. Same goes for shift_intensity=True
and clip_negative_vals=False
👍🏽 Ridge does seem to be useful to speedup the regression. I was also looking into using the regression module of sklearn
and found this comment from the discussion very useful: scikit-learn/scikit-learn#13923 (comment).
Should I use this? I was thinking of adding an option called ols_fast
instead of just replacing ols
with ridge
and a very small alpha
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's very odd that ridge is faster than OLS, given that they essentially are the same algorithm with a small tweak that shouldn't really affect performance at all. But 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep... me too!😂
doc/examples/denoise_patch2self.py
Outdated
@@ -82,6 +90,15 @@ | |||
Please set `shift_intensity=True` and `clip_negative_vals=False` by default to | |||
avoid negative values in the denoised output. | |||
|
|||
The `b0_threshold` is used to separate the b0 volumes from the DWI volumes. | |||
Changing the value of the b0 threshold is needed is the b0 volumes in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the value of the b0 threshold is needed is the b0 volumes in the | |
Changing the value of the b0 threshold is needed if the b0 volumes in the |
doc/examples/denoise_patch2self.py
Outdated
|
||
The default value of `b0_threshold` in DIPY is set to 50. If using data | ||
such as HCP 7T, the b0 volumes tend to have a higher b-value (>=50) | ||
assoiciated with them in the `bval` file. Please check the b-values for b0s and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assoiciated with them in the `bval` file. Please check the b-values for b0s and | |
associated with them in the `bval` file. Please check the b-values for b0s and |
While we are at it: should I also add |
70 tutorials x 5min = 350min =~ 6h more.... so I recommend to pick up not more than 10 tutorials. |
Yes! I don't think it's relevant for all tutorials either :) Here are the ones where I would add it:
Does this sound okay? |
+1 👍🏾 |
Codecov Report
@@ Coverage Diff @@
## master #2382 +/- ##
=======================================
Coverage 85.23% 85.24%
=======================================
Files 126 126
Lines 16575 16575
Branches 2686 2686
=======================================
+ Hits 14128 14129 +1
+ Misses 1762 1759 -3
- Partials 685 687 +2
|
@arokem, Do you want to have a last look? What is the final decision concerning this PR? |
I would only change those tutorials that already have denoising in them, but I think that we can take that on another PR (or even better, series of PRs). I think that this PR is ready to go. |
Thank you @ShreyasFadnavis, thank you for the review @arokem |