search

CEA-COSMIC / pysap-mri

MRI external plugin for Python Sparse data Analysis Package
Other
43 stars 18 forks source link

Gradient operator not initialized in SelfCalibrationReconstructor when sensivity maps are given manually #77

Closed Daval-G closed 4 years ago

Daval-G commented 4 years ago

I am using a SelfCalibrationReconstructor on the same data over and over and I would like to not compute the sensivity maps everytime but save them once and then give them to the reconstructor.

The reconstruct method already has an argument to avoid recomputing the sensivity maps here: https://github.com/CEA-COSMIC/pysap-mri/blob/c39764bf07c8dbefdb60c4e0be92fa3f96b477e2/mri/reconstructors/self_calibrating.py#L129

The problem is that currently the reconstructor does not initialize the gradient operator during its own initialization: https://github.com/CEA-COSMIC/pysap-mri/blob/c39764bf07c8dbefdb60c4e0be92fa3f96b477e2/mri/reconstructors/self_calibrating.py#L111

So, the only way the SelfCalibrationReconstructor can initialize the gradient operator is by letting it recompute the sensivity maps at least once there: https://github.com/CEA-COSMIC/pysap-mri/blob/c39764bf07c8dbefdb60c4e0be92fa3f96b477e2/mri/reconstructors/self_calibrating.py#L174

chaithyagr commented 4 years ago

Marking this as an enhancement for now. @Daval-G are you doing this? Assigning you for now. Also, I was wondering if we could get a verbosity that shows the Smaps after computation? Is this doable in this PR @Daval-G ?

Daval-G commented 4 years ago

I will do a PR to fix that problem soon. Concerning the verbosity feature I don't think it is the job of the reconstructor to do so. But for sure there is a need for a better way to retrieve the Smaps than smaps = reconstructor.extra_grad_args["Smaps"], so it would make sense to either add a documented getter, or simply to return the smaps along with x_final, costs and metrics at the end of the reconstruct method. I prefer the first option.

chaithyagr commented 4 years ago

The latter is surely problematic as that means specifically to this reconstructor we have different output. I agree with a getter function for obtaining Smaps. Maybe a setter function also that re-initilizes the gradient when Smaps is updated?

Daval-G commented 4 years ago

It makes sense to me. I will include the getter and setter in my future PR.

chaithyagr commented 4 years ago

Merged. in #79 , closing