-
Notifications
You must be signed in to change notification settings - Fork 16
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
ENH: Implement method to plot DWI gradient #27
Conversation
Update main from upstream nipreps/EddyMotionCorrection
Hello @sebastientourbier! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-04-12 09:32:43 UTC |
eddymotion/viz.py
Outdated
Draw the vectors on a shell. | ||
|
||
Adapted from the code of Emmanuel Caruyer | ||
(https://github.com/ecaruyer/qspace/blob/master/qspace/visu/visu_points.py) |
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 the way to copy this code over is to include the original license somewhere in this file (you can also additionally include a note in every docstring pointing to the original implementation).
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 believe the BSD license does not mandate that (although, I agree what you propose would be the right thing to do).
I propose:
- Retain the copyright notice at the top of the file Ariel liked, as mandated for redistribution of the original source code by BSD.
- Make a list of the most relevant modifications of the code, as if this were some sort of Apache License.
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'll send a proposal of how to do this as soon as I get the time.
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.
Thanks @oesteban!
It is indeed not clear yet to me how to make it the most clear.
In summary:
- the method
rotation_matrix
is exactly the code of Emmanuel. - the method
draw_circle
has been modified to take the full list of normalized bvecs and corresponding circle radii instead of taking the list of bvecs and radii for a specific shell (bvalue) - the methods
draw_points
has been majorly modified including:- the input is a single 2D numpy array of the gradient table in RAS+B format
- the scaling of the circle radius for each bvec proportional to the inverse of the bvals. A minimum/maximal value for the radii can be specified
- Circles for each bvec are drawn at once instead of looping over the shells.
- Some variables have been renamed (like
vects
tobvecs
)
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.
A few suggestions from a first quick review.
eddymotion/viz.py
Outdated
|
||
def plot_gradients(gradients, | ||
title="Shells reprojected", | ||
figsize=(9.0, 9.0), |
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.
figsize=(9.0, 9.0), | |
ax=None, |
eddymotion/viz.py
Outdated
fig = plt.figure(figsize=figsize) | ||
ax = fig.add_subplot(111, projection='3d') |
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.
fig = plt.figure(figsize=figsize) | |
ax = fig.add_subplot(111, projection='3d') | |
if ax is None: | |
figsize = kwargs.pop("figsize", (9.0, 9.0)) | |
fig = plt.figure(figsize=figsize) | |
ax = fig.add_subplot(111, projection='3d') |
@sebastientourbier I have sent a PR against this branch to address the licensing comments. |
ENH: Licensing and stylistic suggestions
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.
Re-add some suggestions after my PR left them outdated
Okay, I think after my latest suggestions we can go ahead and merge this. WDYT @sebastientourbier ? |
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.
One more little thing I forgot I meant to suggest
ENH: Implement method to plot DWI gradient
ENH: Implement method to plot DWI gradient
ENH: Implement method to plot DWI gradient Former-commit-id: 3ff2e47
Providing the option to visualize the DWI gradient.
The diffusion gradient is displayed on a unit sphere where for each direction the size and color vary depending on its bvalue.
It works as the following:
The notebook can be found in
docs/notebooks
.