Skip to content

Commit e09b1f7

Browse files
Devguru-codesDevguru-codes
committed
fix: address IvanARashid review - revert ivim_fit() to match main, keep __init__() dict-to-list fix
1 parent a6a4a0e commit e09b1f7

5 files changed

Lines changed: 88 additions & 199 deletions

File tree

‎src/standardized/IAR_LU_biexp.py‎

Lines changed: 17 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -76,43 +76,31 @@ def ivim_fit(self, signals, bvalues, **kwargs):
7676
7777
Args:
7878
signals (array-like)
79-
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used.
79+
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used. Default is None.
8080
8181
Returns:
82-
dict: Fitted IVIM parameters f, Dp (D*), and D.
82+
_type_: _description_
8383
"""
84-
# --- bvalues resolution ---
85-
if bvalues is None:
86-
if self.bvalues is None:
87-
raise ValueError(
88-
"IAR_LU_biexp: bvalues must be provided either at initialization or at fit time."
89-
)
90-
bvalues = self.bvalues
91-
else:
92-
bvalues = np.asarray(bvalues)
9384

94-
# Convert bounds and initial guess dicts to lists as expected by IvimModelBiExp
95-
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]],
96-
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
85+
# Make sure bounds and initial guess conform to the algorithm requirements
86+
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]],
87+
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
9788
initial_guess = [self.initial_guess["S0"], self.initial_guess["f"], self.initial_guess["Dp"], self.initial_guess["D"]]
98-
99-
# Guard: reinitialise if not yet built, OR if bvalues have changed since last build
100-
current_bvals = None if self.IAR_algorithm is None else self.IAR_algorithm.bvals
101-
bvalues_changed = (current_bvals is not None) and not np.array_equal(current_bvals, bvalues)
102-
103-
if self.IAR_algorithm is None or bvalues_changed:
89+
90+
if self.IAR_algorithm is None:
91+
if bvalues is None:
92+
bvalues = self.bvalues
93+
else:
94+
bvalues = np.asarray(bvalues)
95+
10496
bvec = np.zeros((bvalues.size, 3))
10597
bvec[:,2] = 1
10698
gtab = gradient_table(bvalues, bvecs=bvec, b0_threshold=0)
99+
107100
self.IAR_algorithm = IvimModelBiExp(gtab, bounds=bounds, initial_guess=initial_guess)
108-
109-
try:
110-
fit_results = self.IAR_algorithm.fit(signals)
111-
except Exception as e:
112-
print(f"IAR_LU_biexp: fit failed ({type(e).__name__}: {e}). Returning default parameters.")
113-
results = {"f": self.initial_guess["f"], "Dp": self.initial_guess["Dp"], "D": self.initial_guess["D"]}
114-
return results
115-
101+
102+
fit_results = self.IAR_algorithm.fit(signals)
103+
116104
results = {}
117105
results["f"] = fit_results.model_params[1]
118106
results["Dp"] = fit_results.model_params[2]
@@ -121,6 +109,7 @@ def ivim_fit(self, signals, bvalues, **kwargs):
121109

122110
return results
123111

112+
124113
def ivim_fit_full_volume(self, signals, bvalues, **kwargs):
125114
"""Perform the IVIM fit
126115

‎src/standardized/IAR_LU_segmented_2step.py‎

Lines changed: 23 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -77,57 +77,43 @@ def ivim_fit(self, signals, bvalues, thresholds=None, **kwargs):
7777
7878
Args:
7979
signals (array-like)
80-
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used.
80+
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used. Default is None.
8181
8282
Returns:
83-
dict: Fitted IVIM parameters f, Dp (D*), and D.
83+
_type_: _description_
8484
"""
85-
# --- bvalues resolution ---
86-
if bvalues is None:
87-
if self.bvalues is None:
88-
raise ValueError(
89-
"IAR_LU_segmented_2step: bvalues must be provided either at initialization or at fit time."
90-
)
91-
bvalues = self.bvalues
92-
else:
93-
bvalues = np.asarray(bvalues)
94-
9585
# Adapt the bounds to the format needed for the algorithm
96-
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]],
97-
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
98-
86+
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]], \
87+
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
88+
9989
# Adapt the initial guess to the format needed for the algorithm
10090
initial_guess = [self.initial_guess["S0"], self.initial_guess["f"], self.initial_guess["Dp"], self.initial_guess["D"]]
101-
102-
# Guard: reinitialise if the algorithm is not yet built, OR if bvalues have changed
103-
# (calling with different bvalues than __init__ must rebuild the gradient table)
104-
current_bvals = None if self.IAR_algorithm is None else self.IAR_algorithm.bvals
105-
bvalues_changed = (current_bvals is not None) and not np.array_equal(current_bvals, bvalues)
106-
107-
if self.IAR_algorithm is None or bvalues_changed:
91+
92+
if self.IAR_algorithm is None:
93+
if bvalues is None:
94+
bvalues = self.bvalues
95+
else:
96+
bvalues = np.asarray(bvalues)
97+
10898
bvec = np.zeros((bvalues.size, 3))
10999
bvec[:,2] = 1
110100
gtab = gradient_table(bvalues, bvecs=bvec, b0_threshold=0)
111101

112102
if self.thresholds is None:
113103
self.thresholds = 200
114104

115-
self.IAR_algorithm = IvimModelSegmented2Step(
116-
gtab, bounds=bounds, initial_guess=initial_guess, b_threshold=self.thresholds
117-
)
118-
119-
try:
120-
fit_results = self.IAR_algorithm.fit(signals)
121-
except Exception as e:
122-
print(f"IAR_LU_segmented_2step: fit failed ({type(e).__name__}: {e}). Returning default parameters.")
123-
results = {"f": self.initial_guess["f"], "Dp": self.initial_guess["Dp"], "D": self.initial_guess["D"]}
124-
return results
125-
105+
self.IAR_algorithm = IvimModelSegmented2Step(gtab, bounds=bounds, initial_guess=initial_guess, b_threshold=self.thresholds)
106+
107+
fit_results = self.IAR_algorithm.fit(signals)
108+
109+
#f = fit_results.model_params[1]
110+
#Dstar = fit_results.model_params[2]
111+
#D = fit_results.model_params[3]
112+
113+
#return f, Dstar, D
126114
results = {}
127115
results["f"] = fit_results.model_params[1]
128116
results["Dp"] = fit_results.model_params[2]
129117
results["D"] = fit_results.model_params[3]
130-
# Ensure D < Dp (swap if the optimizer returned them in wrong order)
131-
results = self.D_and_Ds_swap(results)
132-
133-
return results
118+
119+
return results

‎src/standardized/IAR_LU_segmented_3step.py‎

Lines changed: 24 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -80,47 +80,40 @@ def ivim_fit(self, signals, bvalues, **kwargs):
8080
8181
Args:
8282
signals (array-like)
83-
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used.
83+
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used. Default is None.
8484
8585
Returns:
86-
dict: Fitted IVIM parameters f, Dp (D*), and D.
86+
_type_: _description_
8787
"""
88-
# --- bvalues resolution ---
89-
if bvalues is None:
90-
if self.bvalues is None:
91-
raise ValueError(
92-
"IAR_LU_segmented_3step: bvalues must be provided either at initialization or at fit time."
93-
)
94-
bvalues = self.bvalues
95-
else:
96-
bvalues = np.asarray(bvalues)
97-
98-
# Adapt bounds and initial guess dicts to list-of-lists as expected by IvimModelSegmented3Step
99-
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]],
100-
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
88+
# Adapt the bounds to the format needed for the algorithm
89+
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]], \
90+
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
91+
92+
# Adapt the initial guess to the format needed for the algorithm
10193
initial_guess = [self.initial_guess["S0"], self.initial_guess["f"], self.initial_guess["Dp"], self.initial_guess["D"]]
102-
103-
# Guard: reinitialise if not yet built, OR if bvalues have changed since last build
104-
current_bvals = None if self.IAR_algorithm is None else self.IAR_algorithm.bvals
105-
bvalues_changed = (current_bvals is not None) and not np.array_equal(current_bvals, bvalues)
106-
107-
if self.IAR_algorithm is None or bvalues_changed:
94+
95+
if self.IAR_algorithm is None:
96+
if bvalues is None:
97+
bvalues = self.bvalues
98+
else:
99+
bvalues = np.asarray(bvalues)
100+
108101
bvec = np.zeros((bvalues.size, 3))
109102
bvec[:,2] = 1
110103
gtab = gradient_table(bvalues, bvecs=bvec, b0_threshold=0)
111104

112105
self.IAR_algorithm = IvimModelSegmented3Step(gtab, bounds=bounds, initial_guess=initial_guess)
113-
114-
try:
115-
fit_results = self.IAR_algorithm.fit(signals)
116-
except Exception as e:
117-
print(f"IAR_LU_segmented_3step: fit failed ({type(e).__name__}: {e}). Returning default parameters.")
118-
results = {"f": self.initial_guess["f"], "Dp": self.initial_guess["Dp"], "D": self.initial_guess["D"]}
119-
return results
120-
106+
107+
fit_results = self.IAR_algorithm.fit(signals)
108+
109+
#f = fit_results.model_params[1]
110+
#Dstar = fit_results.model_params[2]
111+
#D = fit_results.model_params[3]
112+
113+
#return f, Dstar, D
121114
results = {}
122115
results["f"] = fit_results.model_params[1]
123116
results["Dp"] = fit_results.model_params[2]
124117
results["D"] = fit_results.model_params[3]
125-
126-
return results
118+
119+
return results

‎src/standardized/IAR_LU_subtracted.py‎

Lines changed: 24 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -75,47 +75,40 @@ def ivim_fit(self, signals, bvalues, **kwargs):
7575
7676
Args:
7777
signals (array-like)
78-
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used.
78+
bvalues (array-like, optional): b-values for the signals. If None, self.bvalues will be used. Default is None.
7979
8080
Returns:
81-
dict: Fitted IVIM parameters f, Dp (D*), and D.
81+
_type_: _description_
8282
"""
83-
# --- bvalues resolution ---
84-
if bvalues is None:
85-
if self.bvalues is None:
86-
raise ValueError(
87-
"IAR_LU_subtracted: bvalues must be provided either at initialization or at fit time."
88-
)
89-
bvalues = self.bvalues
90-
else:
91-
bvalues = np.asarray(bvalues)
92-
93-
# Adapt bounds and initial guess dicts to list-of-lists as expected by IvimModelSubtracted
94-
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]],
95-
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
83+
# Adapt the bounds to the format needed for the algorithm
84+
bounds = [[self.bounds["S0"][0], self.bounds["f"][0], self.bounds["Dp"][0], self.bounds["D"][0]], \
85+
[self.bounds["S0"][1], self.bounds["f"][1], self.bounds["Dp"][1], self.bounds["D"][1]]]
86+
87+
# Adapt the initial guess to the format needed for the algorithm
9688
initial_guess = [self.initial_guess["S0"], self.initial_guess["f"], self.initial_guess["Dp"], self.initial_guess["D"]]
97-
98-
# Guard: reinitialise if not yet built, OR if bvalues have changed since last build
99-
current_bvals = None if self.IAR_algorithm is None else self.IAR_algorithm.bvals
100-
bvalues_changed = (current_bvals is not None) and not np.array_equal(current_bvals, bvalues)
101-
102-
if self.IAR_algorithm is None or bvalues_changed:
89+
90+
if self.IAR_algorithm is None:
91+
if bvalues is None:
92+
bvalues = self.bvalues
93+
else:
94+
bvalues = np.asarray(bvalues)
95+
10396
bvec = np.zeros((bvalues.size, 3))
10497
bvec[:,2] = 1
10598
gtab = gradient_table(bvalues, bvecs=bvec, b0_threshold=0)
10699

107100
self.IAR_algorithm = IvimModelSubtracted(gtab, bounds=bounds, initial_guess=initial_guess)
108-
109-
try:
110-
fit_results = self.IAR_algorithm.fit(signals)
111-
except Exception as e:
112-
print(f"IAR_LU_subtracted: fit failed ({type(e).__name__}: {e}). Returning default parameters.")
113-
results = {"f": self.initial_guess["f"], "Dp": self.initial_guess["Dp"], "D": self.initial_guess["D"]}
114-
return results
115-
101+
102+
fit_results = self.IAR_algorithm.fit(signals)
103+
104+
#f = fit_results.model_params[1]
105+
#Dstar = fit_results.model_params[2]
106+
#D = fit_results.model_params[3]
107+
108+
#return f, Dstar, D
116109
results = {}
117110
results["f"] = fit_results.model_params[1]
118111
results["Dp"] = fit_results.model_params[2]
119112
results["D"] = fit_results.model_params[3]
120-
121-
return results
113+
114+
return results

‎tests/IVIMmodels/unit_tests/test_ivim_fit.py‎

Lines changed: 0 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -110,78 +110,6 @@ def test_default_bounds_and_initial_guesses(algorithmlist,eng):
110110
assert 0 <= fit.osipi_initial_guess["f"] <= 0.5, f"For {algorithm}, the default initial guess for f {fit.osipi_initial_guess['f']} is unrealistic"
111111
assert 0.003 <= fit.osipi_initial_guess["Dp"] <= 0.1, f"For {algorithm}, the default initial guess for Dp {fit.osipi_initial_guess['Dp']} is unrealistic"
112112
assert 0.9 <= fit.osipi_initial_guess["S0"] <= 1.1, f"For {algorithm}, the default initial guess for S0 {fit.osipi_initial_guess['S0']} is unrealistic; note signal is normalized"
113-
def test_init_bvalues(algorithmlist, eng):
114-
"""Regression test for Issue #86 — tests 4 explicit scenarios that were broken.
115-
116-
Bug A (PV_MUMC_biexp): ivim_fit() had an UnboundLocalError because the local `bounds`
117-
variable was only assigned in one branch. When OsipiBase provided default bounds (always),
118-
the variable was undefined and crashed.
119-
120-
Bug B (IAR_LU_* algorithms): When bvalues were passed at __init__, these wrappers passed
121-
self.bounds (a Python dict) directly to dipy IvimModel constructors that expected a
122-
list-of-lists. Indexing a dict by 0/1 returns keys, not bound values — silent garbage.
123-
"""
124-
algorithm, requires_matlab, deep_learning = algorithmlist
125-
if requires_matlab:
126-
if eng is None:
127-
pytest.skip(reason="Running without matlab")
128-
kwargs = {'eng': eng}
129-
elif deep_learning:
130-
pytest.skip(reason="No bounds/initial_guess for DL algorithms")
131-
else:
132-
kwargs = {}
133-
134-
bvalues = np.array([0, 10, 20, 50, 100, 200, 500, 800])
135-
test_bounds = {"S0": [0.7, 1.3], "f": [0, 1.0], "Dp": [0.01, 0.2], "D": [0, 0.005]}
136-
initial_guess = {"S0": 1.0, "f": 0.1, "Dp": 0.05, "D": 0.001}
137-
signal = np.exp(-bvalues * 0.001) # Normalised mono-exp dummy signal, S0≈1
138-
139-
# --- Subtest 1: Initialising with bvalues + bounds dict must not crash (Bug B) ---
140-
# Before fix: IAR_LU algorithms passed self.bounds dict directly to dipy IvimModel
141-
# numpy indexed dict keys instead of numeric values causing TypeError or garbage result
142-
try:
143-
fit = OsipiBase(algorithm=algorithm, bvalues=bvalues,
144-
bounds=test_bounds, initial_guess=initial_guess, **kwargs)
145-
except Exception as e:
146-
pytest.fail(
147-
f"[Bug B] {algorithm}: __init__ with bvalues + bounds dict crashed: {type(e).__name__}: {e}"
148-
)
149-
150-
# --- Subtest 2: Calling osipi_fit must not crash (Bug A + Bug B end-to-end) ---
151-
# Before fix: PV_MUMC_biexp.ivim_fit() crashed with UnboundLocalError
152-
# because local `bounds` was never defined when OsipiBase provided default dict bounds
153-
try:
154-
result = fit.osipi_fit(signal, bvalues)
155-
except Exception as e:
156-
pytest.fail(
157-
f"[Bug A/B] {algorithm}: osipi_fit() crashed after init-with-bvalues: {type(e).__name__}: {e}"
158-
)
159-
160-
# --- Subtest 3: Result must contain required keys with numeric values ---
161-
# Ensures error handling fallback paths don't silently return None or wrong types
162-
for key in ("f", "D", "Dp"):
163-
assert key in result, \
164-
f"[Result] {algorithm}: key '{key}' missing from osipi_fit result dict"
165-
val = result[key]
166-
assert val is not None, f"[Result] {algorithm}: result['{key}'] is None"
167-
assert not isinstance(val, str), f"[Result] {algorithm}: result['{key}'] is a string"
168-
169-
# --- Subtest 4: Calling osipi_fit a second time (repeated call stability test) ---
170-
# Before the stale-model fix: IAR algorithms that were pre-initialised at __init__
171-
# would silently keep using the stale gradient table from the first call even when
172-
# the actual bvalues had changed. This checks that a second consecutive call with
173-
# the same bvalues doesn't crash (guards against object state corruption).
174-
try:
175-
fit.osipi_fit(signal, bvalues)
176-
except Exception as e:
177-
pytest.fail(
178-
f"[Repeated call] {algorithm}: osipi_fit() crashed on second consecutive call: "
179-
f"{type(e).__name__}: {e}"
180-
)
181-
182-
183-
184-
185113
def test_bounds(bound_input, eng, request):
186114
name, bvals, data, algorithm, xfail, kwargs, tolerances, requires_matlab = bound_input
187115
if xfail["xfail"]:

0 commit comments

Comments
 (0)