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

VarPro Fit Example IVIM #1789

Merged
merged 12 commits into from Apr 1, 2019

Conversation

@ShreyasFadnavis
Copy link
Member

commented Mar 16, 2019

  • Add Tests
  • Add Docstrings and Refs
@pep8speaks

This comment has been minimized.

Copy link

commented Mar 16, 2019

Hello @ShreyasFadnavis, Thank you for updating !

Line 85:2: W605 invalid escape sequence '\m'
Line 85:19: W605 invalid escape sequence '\m'
Line 111:21: W605 invalid escape sequence '\m'
Line 111:42: W605 invalid escape sequence '\m'
Line 112:81: E501 line too long (81 > 80 characters)
Line 114:81: E501 line too long (82 > 80 characters)
Line 122:81: E501 line too long (82 > 80 characters)
Line 236:81: E501 line too long (83 > 80 characters)
Line 242:81: E501 line too long (81 > 80 characters)
Line 260:10: W605 invalid escape sequence '\m'
Line 358:4: W292 no newline at end of file

Comment last updated at 2019-03-26 20:11:20 UTC
@codecov-io

This comment has been minimized.

Copy link

commented Mar 17, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@bfde017). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1789   +/-   ##
=========================================
  Coverage          ?   83.88%           
=========================================
  Files             ?      120           
  Lines             ?    14570           
  Branches          ?     2295           
=========================================
  Hits              ?    12222           
  Misses            ?     1824           
  Partials          ?      524
@arokem

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Looks promising. Let me know when this is ready for a review.

For the time being, just a small comment: I don't think that the strings here need to all be raw strings. So, no need for all those r.

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

Looks promising. Let me know when this is ready for a review.

For the time being, just a small comment: I don't think that the strings here need to all be raw strings. So, no need for all those r.

Thank you so much for this, I will take them down and ping you when ready for a review!

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@arokem I guess I am done.. Please can you review?

@ShreyasFadnavis ShreyasFadnavis changed the title [WIP] VarPro Fit Example IVIM VarPro Fit Example IVIM Mar 19, 2019

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@arokem I had added the rs to avoid the \m pep8 that arises due to the math in the example..

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Proposed some edits in ShreyasFadnavis#1

Merge pull request #1 from arokem/ivim_varpro_example-edits
Some suggested edits for the IVIM example.
@arokem

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

OK. I think that this is ready to go. Unless someone objects, I can merge this in a couple of days.

@skoudoro
Copy link
Member

left a comment

Apart from my small comment, it looks good to me too.

t1 = time.time()
ivimfit_vp = ivimmodel_vp.fit(data_slice)
t2 = time.time()
total = t2 - t1

This comment has been minimized.

Copy link
@skoudoro

skoudoro Mar 21, 2019

Member

I think it would be good to print total or to remove total, t1, t2.

It mainly depends on the goal so I'm good with one of this 2 options. Currently, It appears twice and you never really use this variable

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Mar 22, 2019

Author Member

Good catch @skoudoro ... I think I will take it down for now, will print this in a later version after cythonizing a bit!

verticalalignment='center', transform=plt.gca().transAxes)
plt.legend(loc='upper right')
plt.savefig("ivim_voxel_plot.png")
"""

This comment has been minimized.

Copy link
@skoudoro

skoudoro Mar 22, 2019

Member

The rendering will fail here. Can you add an empty line between plt. and """

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Mar 22, 2019

Author Member

Yes, on it..

Let us get the various plots with `fit_method = 'VarPro'` so that we can
visualize them in one page
"""
plot_map(ivimfit_vp.S0_predicted, "Predicted S0", (0, 10000),

This comment has been minimized.

Copy link
@skoudoro

skoudoro Mar 22, 2019

Member

Same here for the empty line between doc and code

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

LGTM, It is ready to be merged, unless someone objects as @arokem said before 😄

@@ -1,4 +1,4 @@
"""
r"""

This comment has been minimized.

Copy link
@arokem

arokem Mar 25, 2019

Member

I don't think that these need all be raw strings

"""

ivim_predict_vp = ivimfit_vp.predict(gtab)[i, j, :]
plt.scatter(gtab.bvals, data_slice[i, j, :],

This comment has been minimized.

Copy link
@arokem

arokem Mar 25, 2019

Member

I just noticed that this adds stuff to the previous figure. I think that you need to add a new figure here, before starting to plot. Also, could you please add to this figure the prediction for this voxel from the 'LM' fit as well?

This comment has been minimized.

Copy link
@arokem

arokem Mar 25, 2019

Member

To clarify, I am getting this

predicted_S0

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Mar 25, 2019

Author Member

Fixing it!

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

Sorry. It looks like that comment was "pending" until now... Do you still get these two plots on top of each other, @ShreyasFadnavis ?

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

Sorry. It looks like that comment was "pending" until now... Do you still get these two plots on top of each other, @ShreyasFadnavis ?

@arokem Let me check once again! That sure is weird!

plt.text(0.65, 0.50, text_fit, horizontalalignment='center',
verticalalignment='center', transform=plt.gca().transAxes)
plt.legend(loc='upper right')
plt.savefig("ivim_voxel_plot.png")

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Mar 25, 2019

Author Member

@arokem Can I just do plt.close() here so that the labels don't overlap as above?

I think that should work as the next plots make use of the plot_map() helper function anyway!

This comment has been minimized.

Copy link
@arokem

arokem Mar 26, 2019

Member

I think that a plt.figure() here should create a new figure and that's where plot_map would then operate. Does that make sense?

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Mar 26, 2019

Author Member

@arokem on it! Sounds good!

@ShreyasFadnavis

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

@arokem @skoudoro Let me know in case of any further issues!

@arokem

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Yes. This now works for me. Separate figures for each bit. I think this is ready to merge. @skoudoro : do you have anything else here? Otherwise, feel free to hit the green button.

@skoudoro skoudoro merged commit 58fccdf into nipy:master Apr 1, 2019

4 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Nothing else to add, Thanks @ShreyasFadnavis. merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.