centralize b-value management at initialization (#155) - #165
Conversation
- Remove bvalues parameter from all 27 algorithm ivim_fit() and ivim_fit_full_volume() signatures in src/standardized/ - All algorithms now access b-values via self.bvalues exclusively - Add **kwargs to algorithms that lacked it (OJ_GU_seg, PV_MUMC, PvH_KB_NKI, TF_reference) - OsipiBase.osipi_fit() and osipi_fit_full_volume() enforce ValueError if bvalues not set at init - Update all test files to pass bvalues at OsipiBase(...) init - Update WrapImage production wrappers to use new API - Remove now-redundant bvalue equality checks in DL algorithms Files modified: 33 (27 algorithms + OsipiBase + 3 tests + 2 wrappers)
This comment was marked as spam.
This comment was marked as spam.
| initial_guess = [self.initial_guess["D"], self.initial_guess["f"], self.initial_guess["Dp"], self.initial_guess["S0"]] | ||
|
|
||
| bvalues=np.array(bvalues) | ||
| bvalues=np.array(self.bvalues) |
There was a problem hiding this comment.
For all algorithms: do we need to set it als bvalues=np.array(self.bvalues)? Or could we not just use self.bvalues instead of bvalues?
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
this comment was either misinterpretted, or ignored. See comment below. But basically, do we need to define bvalues, or can we use self.bvalues directly?
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| "b-values must be provided at initialization (__init__), not at fit-time. " | ||
| "Pass bvalues to the constructor: OsipiBase(bvalues=..., algorithm=...)" | ||
| ) | ||
| use_bvalues = np.asarray(self.bvalues) |
There was a problem hiding this comment.
I think this means we can skip all other np.array/np.asarray
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Otherwise it looks good to me. I'm a little worried that stuf down the line may break, such as the longitudinal testing and the website deployment; these are only run once merged. But let's cross that bridge when we get there. Maybe @IvanARashid can check the osipi_base. |
…IPI#165 review feedback) self.bvalues is already guaranteed to be np.ndarray from OsipiBase.__init__(), so re-wrapping it with np.asarray() or np.array() is a no-op / unnecessary copy. Addresses review feedback from @oliverchampion on PR OSIPI#165. Changes across 25 files: - OsipiBase.osipi_fit(): use_bvalues = self.bvalues (was np.asarray) - OsipiBase.osipi_simple_bias_and_RMSE_test(): bvalues = self.bvalues (was np.asarray) - All 23 standardized algorithms: bvalues = self.bvalues (was np.array/np.asarray)
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
2 participants
fixes #155