centralize b-value management at initialization (#155)#165
centralize b-value management at initialization (#155)#165Devguru-codes wants to merge 3 commits into
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)
|
Hello @oliverchampion sir, please review the changes. If anything needs to be updated, let me know. Thank you. |
| 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?
There was a problem hiding this comment.
i have updated it to self.bvalues.
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?
There was a problem hiding this comment.
I misinterpreted it, my bad.
| "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
There was a problem hiding this comment.
yes, i did that now.
|
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)
|
@oliverchampion sir, i have implemented the suggested changes. please review it. Thank you. |
|
@oliverchampion sir,I have just pushed an update removing all the bvalues = self.bvalues. Since self.bvalues isn't mutated anywhere, replacing the local variable and using self.bvalues directly everywhere worked perfectly. I have checked it on my end. Thank you. |
2 participants
fixes #155