Do merge b0 #37
Do merge b0 #37
Conversation
…begining to avoid conversion to Fortran afterwards; core.py modified to reshape reference signal during NRMSE computation if ActiveAx's singleb0 flag is active
| @@ -59,6 +59,7 @@ def __init__( self, study_path, subject, output_path=None ) : | |||
| self.set_config('doKeepb0Intact', False) # does change b0 images in the predicted signal | |||
| self.set_config('doComputeNRMSE', False) | |||
| self.set_config('doSaveCorrectedDWI', False) | |||
| self.set_config('doMergeB0', True) # Merge b0 volumes | |||
There was a problem hiding this comment.
I think it is better to set it to False by default, this way it won't break the processing so far. Correspondingly, I will do the same in COMMIT for consistency.
There was a problem hiding this comment.
| @@ -224,7 +247,7 @@ def load_kernels( self ) : | |||
| idx_OUT, Ylm_OUT = amico.lut.aux_structures_resample( self.scheme, self.get_config('lmax') ) | |||
|
|
|||
| # Dispatch to the right handler for each model | |||
| self.KERNELS = self.model.resample( self.get_config('ATOMS_path'), idx_OUT, Ylm_OUT ) | |||
| self.KERNELS = self.model.resample( self.get_config('ATOMS_path'), idx_OUT, Ylm_OUT, self.get_config('doMergeB0') ) | |||
There was a problem hiding this comment.
This breaks the code in COMMIT, see here. Can we also apply this strategy of delegating to the model.resample the stripping of the b0s in the KERNELS structure?
| @@ -290,10 +316,6 @@ def fit( self ) : | |||
| if self.scheme.b0_count > 0 : | |||
There was a problem hiding this comment.
Do we need to compute the mean b0 here? As you removed the following lines (293-296) because indeed the normalization is done before once and for all, perhaps we may compute the b0 only later on at line 241 if needed, no?
Also, if the data is normalized, the mean is 1, no? So, in order not to break the following code starting at line 334 (if self.get_config('doSaveCorrectedDWI') :), perhaps we have to calculate and store in a variable the values of the b0s for later usage. What do you think
| @@ -112,17 +112,23 @@ def resample( self, in_path, idx_out, Ylm_out ) : | |||
| Indices of the samples belonging to each shell | |||
| Ylm_out : array | |||
| SH bases to project back each shell to signal space | |||
| doMergeB0: bool | |||
| Merge b0-volumes into a single volume if True | |||
|
|
|||
| Returns | |||
| ------- | |||
| KERNELS : dictionary | |||
| Contains the LUT and all corresponding details. In particular, it is | |||
| required to have a field 'model' set to "self.if". | |||
There was a problem hiding this comment.
[typo] "self.if" should be "self.id"
| @@ -222,31 +228,37 @@ def generate( self, out_path, aux, idx_in, idx_out ) : | |||
| progress.update() | |||
|
|
|||
|
|
|||
| def resample( self, in_path, idx_out, Ylm_out ) : | |||
| def resample( self, in_path, idx_out, Ylm_out, doMergeB0 ) : | |||
There was a problem hiding this comment.
This model is also used in COMMIT, so in case we have to update COMMIT's code as well to account for the modified signature of this resample function call.
- As suggested by daducci#37 (comment) - Allows to use doSaveCorrectedDWI with signal normalization during load_data()
| @@ -45,7 +45,7 @@ def __init__( self, study_path, subject, output_path=None ) : | |||
| self.model = None # set by "set_model" method | |||
| self.KERNELS = None # set by "load_kernels" method | |||
| self.RESULTS = None # set by "fit" method | |||
| self.niiDWI_corrected = None # set by doSaveCorrectedDWI | |||
| @@ -59,6 +59,7 @@ def __init__( self, study_path, subject, output_path=None ) : | |||
| self.set_config('doKeepb0Intact', False) # does change b0 images in the predicted signal | |||
| self.set_config('doComputeNRMSE', False) | |||
| self.set_config('doSaveCorrectedDWI', False) | |||
| self.set_config('doMergeB0', False) # Merge b0 volumes | |||
There was a problem hiding this comment.
Ok, I'll do the same for COMMIT to be consistent
Sets a general preprocessing option called doMergeb0s in
core.CONFIGthat merges the b0 volumes duringload_data()andload_kernels()coherently with COMMIT as suggested by @daducci during a previous pool request.Should close #33, close #35 and close #36 as well as fix other bugs relative to the isExVivo flag in the NODDI.fit() method.