Abdominal phantom #71

oliverchampion · 2024-07-24T06:48:46Z

Describe the changes you have made in this PR

The abdominal phantom has been updated; we need to check whether this influences the automated tests from Eric

Checklist

[ x ] Self-review of changed code
[ x ] Added automated tests where applicable
[ x ] Update Docs & Guides

abdominal phantom up to date

…nted

etpeterson · 2024-07-25T04:16:05Z

I think the remaining piece is to run sim_ivim_sig.py to get the new generic.json and update that. Hopefully the tests will work!

etpeterson · 2024-07-25T04:14:36Z

phantoms/MR_XCAT_qMRI/sim_ivim_sig.py

@@ -261,7 +273,7 @@ def XCAT_to_MR_DCE(XCAT, TR, TE, bvalue, D, f, Ds, b0=3, ivim_cont = True):
        73: 'Pancreas tumor',
    }
    ###############################################################################
-    np.random.seed(42)
+    np.random.seed(41)


Seed change?

This is now fixed. Changed it to test something :).

etpeterson · 2024-07-25T04:15:03Z

utilities/data_simulation/Download_data.py

@@ -2,7 +2,7 @@
 import os
 import subprocess
 import time
-
+import zenodo_get


Is this used?

It is used here:

subprocess.check_call(["zenodo_get", 'https://zenodo.org/records/10696605'])

but as supprocess, it is not recognized that it is used. If you remove it, however, this crashes :).

I don't understand that, but I guess it's not a big deal.

I think this starts a subprocess which then runs the commando in the string (which contains Zenodo_get). But the Zenodo_get function is not recognized by interpreters as it is hidden in a string.
The download happens outside Pycharm, and if I don't put this in a subprocess, it somehow continues the code while downloading, running into errors (using data that still needs downloading).

etpeterson · 2024-08-08T15:27:10Z

I think the remaining piece is to run sim_ivim_sig.py to get the new generic.json and update that. Hopefully the tests will work!

I think this is the remaining piece?

oliverchampion · 2024-08-12T09:51:02Z

I think the remaining piece is to run sim_ivim_sig.py to get the new generic.json and update that. Hopefully the tests will work!

Ah, I had run this, but the generic.json files are on the .gitignore list so nothing was pushed. Is there a good way of pushing these results?
Also, it creates only four "generic_#.json"-files (with # one-four), but not a "generic.json" file.
Are those files manually copied to tests/IVI-models/unit_tests?
Which files are used and which of these is (renamed into?) "generic.json"?

Thanks!

etpeterson · 2024-08-12T15:36:28Z

I think the remaining piece is to run sim_ivim_sig.py to get the new generic.json and update that. Hopefully the tests will work!

Ah, I had run this, but the generic.json files are on the .gitignore list so nothing was pushed. Is there a good way of pushing these results? Also, it creates only four "generic_#.json"-files (with # one-four), but not a "generic.json" file. Are those files manually copied to tests/IVI-models/unit_tests? Which files are used and which of these is (renamed into?) "generic.json"?

Thanks!

Ah, there was a change. I think there should also be a generic_original.json? I think it's being pulled from phantoms/MR_XCAT_qMRI/b_values.json now. I think you should already have that, so rename that generic.json and overwrite the original and git add -f generic.json. The -f forces it to add even though it's ignored.

Then hopefully it all works!

etpeterson

Just the generic.json overwrite remaining.

oliverchampion · 2024-08-13T15:32:19Z

Good news: File is updated (as well as the other generic files).
Bad news: (some) tests fail... But a lot pass... Will need to check this in more detail tomorrow...

oliverchampion · 2024-08-14T08:40:58Z

@etpeterson It seems like only your algorithm is failing, and only in a limited set of tissues. In particular, it has trouble estimating an f of 1 in vessels. But also has some challenges in the pericardium...

I think it has to do with the fact that all algorithms use real tolerances, but yours uses dynamic tolerances. Hence, your tolerances are way more strict (and realistic) than for the other algorithms.

I think the dyn_tolerances are related to an issue/feature request I wrote earlier today :) and should be the way to go.

etpeterson · 2024-08-20T04:10:14Z

@etpeterson It seems like only your algorithm is failing, and only in a limited set of tissues. In particular, it has trouble estimating an f of 1 in vessels. But also has some challenges in the pericardium...

I think it has to do with the fact that all algorithms use real tolerances, but yours uses dynamic tolerances. Hence, your tolerances are way more strict (and realistic) than for the other algorithms.

I think the dyn_tolerances are related to an issue/feature request I wrote earlier today :) and should be the way to go.

I'm not too sure why it's failing now, but I'm not too worried. You could mark those tests as expected failures. So add the regions to this area of algorithms.json. That's probably easiest, especially since it seems like more bounds changes are coming. Actually, looks like Vein was failing and it's now passing, so just swap out "Blood RV" for "Vein" and it should work.

"ETP_SRI_LinearFitting": {
        "xfail_names": {
            "Vein": true
        },

oliverchampion · 2024-08-20T13:49:28Z

Features updated:
1: Phantom is smaller (done previously)
2: signal in phantom can now optionally be T1/T2 weighted. Default has T1/T2 weighting turned off.

@etpeterson It seems like only your algorithm is failing, and only in a limited set of tissues. In particular, it has trouble estimating an f of 1 in vessels. But also has some challenges in the pericardium...
I think it has to do with the fact that all algorithms use real tolerances, but yours uses dynamic tolerances. Hence, your tolerances are way more strict (and realistic) than for the other algorithms.
I think the dyn_tolerances are related to an issue/feature request I wrote earlier today :) and should be the way to go.

I'm not too sure why it's failing now, but I'm not too worried. You could mark those tests as expected failures. So add the regions to this area of algorithms.json. That's probably easiest, especially since it seems like more bounds changes are coming. Actually, looks like Vein was failing and it's now passing, so just swap out "Blood RV" for "Vein" and it should work.
"ETP_SRI_LinearFitting": {
        "xfail_names": {
            "Vein": true
        },

Ah! Now I understand the XPASS fail, haha. I was wondering what that was doing :)

etpeterson · 2024-08-20T14:53:23Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

@@ -169,6 +169,8 @@ def test_ivim_fit_saved(name, bvals, data, algorithm, xfail, kwargs, tolerances,
    tolerances = tolerances_helper(tolerances, ratio, data["noise"])
    [f_fit, Dp_fit, D_fit] = fit.osipi_fit(signal, bvals)
    npt.assert_allclose(data['f'], f_fit, rtol=tolerances["rtol"]["f"], atol=tolerances["atol"]["f"])
-    npt.assert_allclose(data['D'], D_fit, rtol=tolerances["rtol"]["D"], atol=tolerances["atol"]["D"])
-    npt.assert_allclose(data['Dp'], Dp_fit, rtol=tolerances["rtol"]["Dp"], atol=tolerances["atol"]["Dp"])
+    if data['f']<0.80: # we need some signal for D to be detected


Just looking at code in more detail, would this be a relative tolerance thing rather than an if?

Yes, I am doing that in my next project. But either way it seems silly to quantity something that is undefined (D* is f=0 and D if f=1)

Understood. Looks good to me.

oliverchampion and others added 2 commits July 24, 2024 08:44

Merge pull request #70 from OSIPI/main

77fe390

abdominal phantom up to date

updated abdominal phantom to include more tissue and be properly orie…

0ce1638

…nted

etpeterson reviewed Jul 25, 2024

View reviewed changes

Update sim_ivim_sig.py

af52229

etpeterson requested changes Aug 12, 2024

View reviewed changes

updated generic.json files in test

a5267de

adjusted some of the values for tissue with no signal

d76735f

updated unit test files to be normalized SNR-wise

c45231d

change ignore file

a254fc0

etpeterson requested changes Aug 20, 2024

View reviewed changes

etpeterson approved these changes Aug 20, 2024

View reviewed changes

etpeterson merged commit 44051e4 into main Aug 21, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abdominal phantom #71

Abdominal phantom #71

oliverchampion commented Jul 24, 2024

etpeterson commented Jul 25, 2024

etpeterson Jul 25, 2024

oliverchampion Aug 7, 2024

etpeterson Jul 25, 2024

oliverchampion Aug 7, 2024

etpeterson Aug 8, 2024

oliverchampion Aug 12, 2024

etpeterson commented Aug 8, 2024

oliverchampion commented Aug 12, 2024

etpeterson commented Aug 12, 2024

etpeterson left a comment

oliverchampion commented Aug 13, 2024

oliverchampion commented Aug 14, 2024 •

edited

Loading

etpeterson commented Aug 20, 2024 •

edited

Loading

oliverchampion commented Aug 20, 2024

etpeterson Aug 20, 2024

oliverchampion Aug 20, 2024

etpeterson Aug 20, 2024

Abdominal phantom #71

Abdominal phantom #71

Conversation

oliverchampion commented Jul 24, 2024

Describe the changes you have made in this PR

Checklist

etpeterson commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpeterson commented Aug 8, 2024

oliverchampion commented Aug 12, 2024

etpeterson commented Aug 12, 2024

etpeterson left a comment

Choose a reason for hiding this comment

oliverchampion commented Aug 13, 2024

oliverchampion commented Aug 14, 2024 • edited Loading

etpeterson commented Aug 20, 2024 • edited Loading

oliverchampion commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverchampion commented Aug 14, 2024 •

edited

Loading

etpeterson commented Aug 20, 2024 •

edited

Loading