search

OSIPI / TF2.4_IVIM-MRI_CodeCollection

OSIPI TF2.4: IVIM MRI code collection
Apache License 2.0
9 stars 27 forks source link

Update testing b-values with a more realistic set #10

Closed etpeterson closed 6 months ago

etpeterson commented 1 year ago

They're currently skewed high, so should be updated with literature supported values.

kailash360 commented 6 months ago

Hello @etpeterson

I would like to work on this issue.

etpeterson commented 6 months ago

Hi @kailash360! There isn't much detail here I realize. The idea behind this is that we want some good default sets. We have a hard coded set here, but there should be a few based on which body part is being focused on and the exam duration. There is an aspect of searching the literature for these values, so if you aren't familiar with that, one of use could come up with the values and you could add the code. The goal being to have a few sets available for users to call by name.

If that sounds interesting, we could figure this out in more detail. Just let me know.

etpeterson commented 6 months ago

Hi @AhmedBasem20, I'll respond to the pull request here so everyone sees. Unfortunately swapping those out isn't that easy. All the numbers in that file are generated based on the bvalues so we'd need to generate new versions of those files. We recently removed the raw data from the repository and are moving it to zenodo so I'm not sure if that's something we can do immediately or perhaps in a week or so.

I was thinking that in the meantime we could find several reasonable sets, perhaps targeting brain, liver, and muscle. And add them more as reference values that we could use within the repository. From there we could do the next step to regenerate the testing files with all the new reference sets.

Also, @kailash360 is also interested in participating so I'd like to give everyone the opportunity. Maybe a good split would be how I outlined it above, or we could think more about splitting each of those, or perhaps other projects. @oliverchampion do you have thoughts on this?

etpeterson commented 6 months ago

Oh, also: @kailash360 and @AhmedBasem20 you're GSoC students, please join our slack as well!

AhmedBasem20 commented 6 months ago

Thanks @etpeterson for the detailed reply.

I was thinking that in the meantime we could find several reasonable sets, perhaps targeting brain, liver, and muscle. And add them more as reference values that we could use within the repository.

I'm trying to find how and where to update those sets. I see that the bvalues is filled this way: https://github.com/OSIPI/TF2.4_IVIM-MRI_CodeCollection/blob/a9cc38846b304216c437c008a35df81d2f10125f/tests/IVIMmodels/data/test_GenerateData.py#L11

on several parts of the code, I guess if we replaced np.linspace(0, 1000, 11) with our defined bvalues set, the tests will be regenerated based on it, what do you think about this?

Anyways, I'm currently trying to run the project so that I can test that everythings works locally before pushing any code.

etpeterson commented 6 months ago

The generic.json file was created with this script. What we're missing now are the data files used to generate it. I inquired about the new data location, all I know currently is that it's somewhere on Zenodo. Until we get the data there's not much we can do here.

What you're finding are places where our lack of predefined bvalue sets are being filled in with linspaces. If you create a few sets in a file that we could perhaps call by name, that would be a good start.

For example, something like this to replace what you found. Note this is just an example and the actual implementation could vary.

from utilities.bvalues.presets import Brain

pytest.param(0, Brain.number(11), id='0')

I could imagine sets from papers could be added and referred to similarly as well. from utillities.bvalues.presets import <refname>

Maybe starting with 5 presets for now - at least the framework would make more easy to add. Does something like that sound good for a first contribution?

AhmedBasem20 commented 6 months ago

This sounds good to me! Will give it a try and let you know, Thanks.

etpeterson commented 6 months ago

@kailash360 Let me know if you're still interested and we can definitely find something for you.

kailash360 commented 6 months ago

Sure @etpeterson, if there is some other issue I would love to work on it.

Unique-Usman commented 6 months ago

@etpeterson, if there is other issue I can also work. Kindly let me know. Thanks

etpeterson commented 6 months ago

@kailash360 @Unique-Usman I created a few more issues in the repo. Take a look and see if anything looks interesting. Also, if you have other thoughts feel free to suggest them!

oliverchampion commented 6 months ago

@etpeterson thanks for checking this. Yes, I think it makes sense if one student works on this and the other works on another issue. Alternatively, we could have both work on this issue, but we will only merge 1 pull request.

The data is at https://zenodo.org/records/10696605

If someone could check my merge request, the data should be downloaded automatically whenever it is needed in the code.