Parallel #113

oliverchampion · 2025-07-29T20:48:17Z

Describe the changes you have made in this PR

Added parallel computing and testing
Added volume fitting and testing
Added wrapper code
Added SUPER-IVIM-DC fit methods (by accident in this PR)

Link this PR to an issue [optional]

Fixes #ISSUE-NUMBER

Checklist

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

Testing still failing as parallel is not going faster.

Also added technion unit tests and code by acident on this branch.

This failed a specific test (4 organs, but passes rest).

…deCollection into parallel

…oundaries

oliverchampion · 2025-08-06T08:30:52Z

This is now running and ready for review :)

etpeterson

Quick look for now. Just a few comments - looking good.

etpeterson · 2025-08-06T15:01:08Z

phantoms/MR_XCAT_qMRI/sim_ivim_sig.py


 ##########
 # code written by Oliver J Gurney-Champion
 # code adapted from MAtlab code by Eric Schrauben: https://github.com/schrau24/XCAT-ERIC
 # This code generates a 4D IVIM phantom as nifti file

 def phantom(bvalue, noise, TR=3000, TE=40, motion=False, rician=False, interleaved=False,T1T2=True):
+    download_data()


This downloads data during testing now, correct? Is that slow or burdensome?

Yes and no. The volume, parallel and AI tests just last long, full stop. Downloading the data is relatively quick compared to that less than minute Vs hours).

I think there are two ways we can go here.

I think I could accelerate the volume fit by dramatically reducing the number of vowels; there probably is no need for high resolution.

For the parallel fit, the smaller I make the number of vowels/volume, the less of an acceleration we get from going parallel and the harder it is to test this. For small volumes, typically linear goes way faster. That is why I ended up increasing the volume.

So either we can make a large volume (then the download time is neglectable) and do long tests. Or, we can go for a small volume and ignore the parallel timing. In that cases precalculting the volume and saving it will be faster.

That said, the AI tests also last 5+ minutes per algorithm and we must decide whether we want that to be done every merge...

Either way, all building blocks are here now :)

After discussion --> this download only occurs once, so I think it is solved?

etpeterson · 2025-08-06T15:03:28Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

@@ -152,6 +156,83 @@ def test_bounds(bound_input, eng):
            assert passf, f"Fit still passes when initial guess f is out of fit bounds; potentially initial guesses not respected for: {name}"
            assert passDp, f"Fit still passes when initial guess Ds is out of fit bounds; potentially initial guesses not respected for: {name}" '''

+def test_volume(algorithmlist,eng, threeddata):


Similar to the download comment, would it be good to tag these tests as something so people could skip the download during testing? Just a thought - maybe they deserve their own tags or none at all.

Let's discuss :)

etpeterson · 2025-08-06T15:06:50Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

+        assert np.allclose(fit_result['D'], fit_result2['D'], atol=1e-4), "Results differ between parallel and serial"
+        assert np.allclose(fit_result['f'], fit_result2['f'], atol=1e-2), "Results differ between parallel and serial"
+        assert np.allclose(fit_result['Dp'], fit_result2['Dp'], atol=1e-2), "Results differ between parallel and serial"
+    if time_parallel * 1.3 > time_serial:


Time can be tricky because it can vary based on system load. Probably a good choice that's it's not a requirement but this message probably won't ever get seen. I wonder if there's some report we could write to - perhaps in the future - for algorithm profiling purposes?

Good idea! Let's discuss. I did give it a personalised tag, so it is findable. However, it does not appear on the final report at the end of testing.

I've now tried to:

turn off all warnings

turn back on my specific warning

summarize warnings with -r w

Let's see what it does. This may be undesirable as it suppresses all other warnings. But it will highlight the specific warnings we want

oliverchampion · 2025-08-29T15:17:31Z

I've reduced data size and now tests should go much faster!

etpeterson

Looks good to me

src/wrappers/OsipiBase.py

IvanARashid · 2025-09-11T07:37:31Z

src/original/ETP_SRI/LinearFitting.py

@@ -82,11 +83,17 @@ def ivim_fit(self, bvalues, signal):
                # print(Dp_prime)

            if np.any(np.asarray(Dp_prime) < 0) or not np.all(np.isfinite(Dp_prime)):
-                print('Perfusion fit failed')
+                warnings.warn('Perfusion fit failed',


This applies to all the edited files in src/original. I don't think we should touch these. If there's something wrong, it should be on the authors to fix. Or we just accept it as it is and if it is flawed, then it should fail the test in question.

I also don't quite get why this was needed here. If the b-value inputs are not correct, then it should be checked in the wrapper (something I have wanted to do for a while but never got around to)

I think I changed this as the print kept giving output into my prompt. By handling it as a warning, I can suppress the output from my testing reports. They otherwise got very long and hard to find the info I was looking for.

Agreed that we should normally not touch them! @etpeterson, can you accept those changes to your source file?

src/standardized/IAR_LU_biexp.py

oscarjalnefjord · 2025-09-09T07:50:33Z

src/standardized/IAR_LU_biexp.py

-
-        fit_results = self.IAR_algorithm.fit(signals)
+        b0_index = np.where(bvalues == 0)[0][0]
+        mask = signals[...,b0_index]>0.01


0.01 does in some way assume that the signal has been normalized to 1, right? Would it be enough with ">0"?

This applies to src/standardized/OGC_AmsterdamUMC_Bayesian_biexp.py as well

Yeah, the mask was added as it took shorter to fit and I think both approaches were not happy with empty voxels.
For Bayesian approach, actually, one would also want to remove noisy background voxels, as you dont want them to contribute to the data-driven prior... But that is a different story for which I don't think a general fix is possible

oscarjalnefjord · 2025-09-09T08:00:18Z

src/standardized/IAR_LU_biexp.py

-        fit_results = self.IAR_algorithm.fit(signals)
+        b0_index = np.where(bvalues == 0)[0][0]
+        mask = signals[...,b0_index]>0.01
+        fit_results = self.IAR_algorithm.fit(signals, mask=mask)


Is mask a valid kwarg?

yes! Otherwise the testing would fail :)

oscarjalnefjord · 2025-09-09T08:14:31Z

tests/IVIMmodels/unit_tests/algorithms.json

+        "TCML_TechnionIIT_lsqlm",
+        "TCML_TechnionIIT_lsqtrf",
+        "TCML_TechnionIIT_lsq_sls_lm",
+        "TCML_TechnionIIT_SLS",


Two algorithms from TCML missing (BOBYQA and lsq_sls_trf)

Yes, as far as I remember, they are not properly implemented in the source code.

oscarjalnefjord · 2025-09-09T08:22:36Z

tests/IVIMmodels/unit_tests/test_ivim_fit.py

+        assert np.shape(fit_result['D'])[2] == np.shape(data)[2]
+        # check if right variable is in right place
+        assert np.nanmean(fit_result['D']) < np.nanmean(fit_result['Dp'])
+        assert np.nanmean(fit_result['Dp']) < np.nanmean(fit_result['f'])


Should this always be true?

For the test data I provide, this should be true. This is predominantly testing whether we pass on the right output index to the right parameters.

also added some documentation to base

oliverchampion · 2025-09-16T13:54:37Z

@IvanARashid @oscarjalnefjord this should now be fixed.
Ivan, I also added documentation to the main functions in OSIPI_base (Sorry, probably should have made a new PR, but I missed the documentation in volume fitting, and from there it propogated to single voxel and OSIPI base).
POssibly we should add this for more functions...

oscarjalnefjord

Looks fine to me

oliverchampion and others added 14 commits July 25, 2025 09:38

added parallel, testing and 3D testing

faea568

implemented parallel + testing; 3D + testing

a0ad54a

Testing still failing as parallel is not going faster.

fixed bugs

bc69159

last updates

3e446fe

typo

ee96f11

Update conftest.py

0c1067e

updated all typo's and errors. passes tests locally now

cace5b1

Also added technion unit tests and code by acident on this branch.

SLS TRF removed

23e711c

This failed a specific test (4 organs, but passes rest).

download data in right location

bdcd9b9

Merge branch 'main' into parallel

076b776

Update conftest.py

fdc74db

Merge branch 'parallel' of https://github.com/OSIPI/TF2.4_IVIM-MRI_Co…

1e3cecd

…deCollection into parallel

Update test_ivim_fit.py

d64fd6e

test on 2 cores (default avail on github) and with less strict time b…

2f4174c

…oundaries

oliverchampion requested review from IvanARashid and etpeterson August 1, 2025 13:22

oliverchampion added 5 commits August 4, 2025 10:51

fix some of the failed tests and reduce output

aeee141

resolved some of the failing tests

270b95d

fix typos

ae2bd26

remove hard fail on not making parallle faster. Just a warning now.

127d966

hope it does not trigger the AI fits. They are not triggered locally...

1c6d6f2

etpeterson requested changes Aug 6, 2025

View reviewed changes

oliverchampion added 4 commits August 8, 2025 09:42

summarize warnings

04a19d3

Update unit_test.yml

1e9e2c6

Update unit_test.yml

bd265c4

reduce test time

2c10cbd

oliverchampion and others added 2 commits August 29, 2025 17:22

Merge branch 'main' into parallel

0b7af8f

update

51bd03a

oliverchampion requested review from etpeterson and oscarjalnefjord September 5, 2025 14:27

etpeterson approved these changes Sep 9, 2025

View reviewed changes

IvanARashid requested changes Sep 11, 2025

View reviewed changes

oscarjalnefjord reviewed Sep 15, 2025

View reviewed changes

fixed for review

a75d9c0

also added some documentation to base

oscarjalnefjord approved these changes Sep 16, 2025

View reviewed changes

IvanARashid approved these changes Sep 17, 2025

View reviewed changes

oscarjalnefjord merged commit 57b2aa1 into main Sep 17, 2025
9 checks passed

Parallel #113

Parallel #113

Uh oh!

Conversation

oliverchampion commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the changes you have made in this PR

Link this PR to an issue [optional]

Checklist

Uh oh!

oliverchampion commented Aug 6, 2025

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverchampion Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverchampion commented Aug 29, 2025

Uh oh!

etpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverchampion commented Sep 16, 2025

Uh oh!

oscarjalnefjord left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oliverchampion commented Jul 29, 2025 •

edited

Loading

oliverchampion Aug 8, 2025 •

edited

Loading