Add Reconstructors and API for easier MR reconstruction #57
Conversation
chaithyagr
force-pushed
the
mv_reconstructors
branch
from
9b59b5b to
cebed14
Compare
|
@zaccharieramzi , I have taken in most of the review comments in latest push, or have placed my opinions / comments / asked for help. |
mri/reconstructors/base.py
Outdated
| kspace_loc: np.ndarray | ||
| the mask samples in the Fourier domain. | ||
| uniform_data_shape: tuplet | ||
| the shape of the matrix containing the uniform data. |
There was a problem hiding this comment.
Isn't it just data shape? or rather image shape if we don't want to confuse with k-space measurements?
mri/reconstructors/base.py
Outdated
| verbosity level for debug, please check dervied class for details | ||
| on verbosity levels | ||
| """ | ||
| def __init__(self, kspace_loc, uniform_data_shape, n_coils, |
There was a problem hiding this comment.
Why do you think it makes the code less readable? Maybe you mean the docs?
If you look at libraries like for example matplotlib, they have to do this all the time because their objects are so deeply derived or reused, they can't afford to explicitly expose all the arguments to every function.
Look at for example the docs to the plot function: https://matplotlib.org/3.1.1/api/_as_gen/matplotlib.pyplot.plot.html
You can see that they have a huge amount of kwargs (here the docs are super nice because they also list the kwargs directly and automatially but generally it's fine to just point to the corresponding function or class). This eases the maintaining of the docs and code because when you change a function somewhere you don't need to change the docs and code everywhere.
| """ | ||
|
|
||
| # Package import | ||
| from mri.reconstruct.cost import DualGapCost, GenericCost | ||
|
|
There was a problem hiding this comment.
This is direct copy, the earlier movewas done to a shortcut file, (which just had imports)
|
In my opinion, all required coding is done, and I expect the tests to pass. Added back @zaccharieramzi for review. Please do yell out if I missed anything. |
|
I think I have covered mostly all the comments. @LElgueddari can you please do a final review and check if I missed something? |
There was a problem hiding this comment.
Some documentation on the gradient arguments and the regularizer_op to be put in the kwargs. I would even be in favor of putting the other ops in the kwargs but I understand they are used for some other things in the derived classes so I am not against their current state.
| **kwargs : Extra keyword arguments for gradient initialization. | ||
| Please refer to mri.operators.gradient.base for information | ||
| """ | ||
| def __init__(self, fourier_op, linear_op=None, regularizer_op=None, |
There was a problem hiding this comment.
regularizer_op can also be part of the kwargs
There was a problem hiding this comment.
True for the other classes.
| operator and adjoint operator. For wavelets, this can be object of | ||
| class WaveletN or WaveletUD2 from mri.operators . | ||
| If None, sym8 wavelet with nb_scale=3 is chosen. | ||
| regularizer_op: operator, (optional default None) |
There was a problem hiding this comment.
it's not in the params anymore
There was a problem hiding this comment.
Im confused where to have this, can you please help me here? PLease refer to my next comment
|
Tagging @LElgueddari for final merge, if its good for her.. |
This is the final PR in #36 .
Hopefully this completes the Code refactoring process.
I have moved certain utils around to more sensible places.
@zaccharieramzi , Majorly in this PR I have added new reconstructors in mri/reconstructors, which will need maximum reviewing.