CEA-COSMIC / pysap-mri

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

`sparse_rec_fista` fails when using `pywt` wavelets #3

Closed zaccharieramzi closed 5 years ago

zaccharieramzi commented 5 years ago

When trying to use sparse_rec_fista with pywt it fails with:


~/workspace/pysap/pysap/numerics/reconstruct.py in sparse_rec_fista(gradient_op, linear_op, prox_op, cost_op, mu, nb_scales, lambda_init, max_nb_of_iter, atol, metric_call_period, metrics, verbose)
     85     # Check inputs
     86     start = time.clock()
---> 87     if not linear_op.transform.__is_decimated__:
     88         warnings.warn("Undecimated wavelets shouldn't be used with FISTA: "
     89                       "non optimal solution.")

AttributeError: 'Db4' object has no attribute '__is_decimated__'

This is because of the lines where we check whether the wavelet is decimated. We need to adapt these lines to not fail in the pywt case.

philouc commented 5 years ago

We should probably set the attribute __is_decimated__ = False depending on the padding option in use in pywt, shouldn't we?

zaccharieramzi commented 5 years ago

I don't understand why it depends on the padding option.

I actually think all the wavelets in pywt are decimated when we use the classical wavedecn. I say that because I was reading in the docs that they specify that to use an undecimated wavelet, you need to use the stationary wavelet.

philouc commented 5 years ago

Ok so this means that we should set is_decimated = True if the classical wavedecn is used.

zaccharieramzi commented 5 years ago

In my understanding yes. I am submitting a PR for it as soon as possible.

zaccharieramzi commented 5 years ago

So actually the problem is that there are 2 different kinds of attributes at play here:

I think the right way to go is to use the instance attribute and set it when we compute the coefficients (that is when we know whether the wavelet is decimated or not) in decimated or undecimated for the isap wavelets.

@AGrigis I see you coded that part, maybe you have thoughts on this ?

zaccharieramzi commented 5 years ago

So actually, the instance attribute (is_decimated) is supposed to be set in _set_transformation_parameters for the isap wavelets.

However this function is never called. It could be called when setting the data but since use_wrapping is always False when you have pysparse, it never does.

I think it should be called during the initialisation I don't see why it should only be called when setting the data and we don't have pysparse

zaccharieramzi commented 5 years ago

I think for the current time, we should just get rid of this check. People should be aware of what they are using and sometimes you might want to use an undecimated wavelet even in FISTA (it's going to be my case, computing the prox iteratively).

philouc commented 5 years ago

I agree with you. Go ahead and get rid of this check for the moment.

zaccharieramzi commented 5 years ago

Closed by #17