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

Actual S0 for DTI data #1148

Closed
etpeterson opened this Issue Nov 11, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@etpeterson
Contributor

etpeterson commented Nov 11, 2016

Hey @arokem didn't you have some code to calculate the actual b=0 intercept from the DTI fit? Did that make it into the repo somewhere? I want to compare IVIM and DTI fits but I need the actual intercept for the DTI fit.

@arokem

This comment has been minimized.

Member

arokem commented Nov 11, 2016

It's in this unmerged PR:#1036

We decided not to merge it, because it would incur a performance regression. It occurs to me that we might still want to integrate that into our code base, even if we don't require it be used in every case that DTI is fit.

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented Nov 11, 2016

Thanks, that explains why I couldn't find it.

For context I was going to try to fit the DTI and IVIM models and then compare them to see which regions has a better quality fit, and I also wanted to implement a faster double linear IVIM fit using the DTI routines. But in both cases I need the S0 value. I ended up using polyfit like the IVIM code does but I think it makes sense to leverage the DTI implementation in those cases. Is there any reason to use polyfit over DTI other than it just doesn't return the S0 value?

@arokem

This comment has been minimized.

Member

arokem commented Dec 1, 2016

Sorry for the slowness here. I am not sure that I understand that last sentence. Can you paraphrase?

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented Dec 1, 2016

Basically I'm wondering about the advantages or disadvantages of using the DTI fit compared with polyfit. I'm leaning towards DTI because it already has a lot of options but if it's to slow or if the S0 isn't going to be included then maybe polyfit is the better choice. Do you have thoughts/comments on the plusses or minuses for either method?

@arokem

This comment has been minimized.

Member

arokem commented Dec 5, 2016

DTI is going to be rather fast, compared to doing polyfit in every voxel, because you can fit it in one fell swoop for your entire volume. You can infer S0 either from the fit, or (as we do in our prediction) from the measured S0 volumes (assuming you have measured S0).

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Dec 5, 2016

well, for @etpeterson's mentioned aim, you need to infer S0 from the DTI's fit.

Some months ago I had a meeting with @arokem and @Garyfallidis to discuss how to incorporate the S0 fit in DTI's fit class instance. My first suggestion was to add S0 as a model parameter (e.g. adding it as the last element of model_params). However, since this small change will mess with all other Dipy models that depend on DTI, we explored other alternatives such as estimating S0 after the parameters fit (suggestions at https://gist.github.com/arokem/508dc1b22bdbd0bdd748).

Although we decided not to carry with these changes directly into the class fit instance (since it was reducing speed performance), you can infer S0 from DTI posteriorly from DTI model_params and the data using the mathematical proof and code at https://gist.github.com/arokem/508dc1b22bdbd0bdd748.

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented Dec 5, 2016

Thanks guys. I think I'd like to use DTI but I do need the S0 from the fit so it looks like I'm bringing this up again.

I see S0 is calculated in at least OLS and WLS but discarded so maybe a flag to save it? I'm guessing you already thought of this, but maybe as an optional input to TensorFit?

@arokem

This comment has been minimized.

Member

arokem commented Dec 6, 2016

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented Dec 6, 2016

OK, sounds good to me. I'll explore a little and see if what I'm envisioning is possible.

@sahmed95

This comment has been minimized.

Contributor

sahmed95 commented Dec 7, 2016

Talking about measuring performance change over time, I recently came across airspeed - a tool for benchmarking Python software projects over their lifetime. From the Scipy talk :

As software projects mature and become more robust against bugs, they may also lose some of their runtime performance and memory efficiency. Airspeed velocity (asv) is a new tool to help find those performance degradations before they get out to end users. It automatically runs a benchmark suite over a range of commits in a project's repository, as well as in a matrix of configurations of Python versions and other dependencies. The results, possibly from multiple machines, are then collated and published in a web-based report.

@arokem Is this something that can be helpful while tracking performance regression ? I can open a new issue if this seems to be something we would like to do in future.

@etpeterson

This comment has been minimized.

Contributor

etpeterson commented Dec 7, 2016

Alright, I've had a productive few days and managed to get this together: https://github.com/etpeterson/dipy

It seems like it's working and the speed is similar, only increasing by 50ms for a total fit time around 3s but the size does increase from about 1.56GB to 1.7GB. But that's just with the option turned off vs turned on whereas the real test is vs master as well.

I still need to add more tests, documentation, and come up with a better speed and memory comparison. I also have IVIM edits in there as well, so I should strip those out for a PR.

Of course I'm happy to hear thoughts about this.

@arokem

This comment has been minimized.

Member

arokem commented Dec 7, 2016

@RafaelNH

This comment has been minimized.

Contributor

RafaelNH commented Dec 8, 2016

Yes, please do a PR (or two PRs - one for DTI and another for IVIM)! Would love to review it!

@arokem

This comment has been minimized.

Member

arokem commented Dec 16, 2016

I guess I can close this one. Please reopen if this still needs to be here.

@arokem arokem closed this Dec 16, 2016

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