search

JuliaImageRecon / RegularizedLeastSquares.jl

MIT License
20 stars 10 forks source link

PR #48 and Integration with MRIReco #49

Closed JAgho closed 1 year ago

JAgho commented 1 year ago

Hi All,

Just writing regarding some playing I've been doing with the 3D TV implementation in PR #48. Since my recon is necessarily 3D, getting this working was a priority. I found that only a small change was needed to ensure the code dispatched correctly. Namely, line 55 was changed to:

return proxTV!(x, λ, shape, dims)#; kwargs...) # define shape and dims w/o kwargs to enable multiple dispatch on dims

As far as I can tell, the kwargs passed don't actually do anything at present - I guess they could be used to set e.g. the number of TV iterations or similar.

I can see from the discussion that the decision regarding what to do with the kwargs is still open, and I think my understanding of the structure of the MRIReco / RLS mix is a little limited still.

Please, let me know if there's any testing that I can do that would help

tknopp commented 1 year ago

I merged #45, which includes #48. But instead of removing the kwargs in line 55 I added kwargs to the lower functions in the stack. Could you look if that works? I will then soon do a release when we resolved that.

JakobAsslaender commented 1 year ago

@tknopp: Any chance we can spell out the keyword arguments instead of using kargs? I really don't like kargs, as it makes it very intransparent in what arguments you can use, and if you call it with invalid arguments, it does not throw an error, so you don't really what is going on. This makes, IMHO, the NFFT.jl package really hard to us and this is a shame because it is such a powerful and well-written package!

tknopp commented 1 year ago

I can't right now really give a qualified answer to the question if kargs are needed here or not. They were introduced by you in https://github.com/tknopp/RegularizedLeastSquares.jl/blob/master/src/proximalMaps/ProxTV.jl#L55 if I see it right. My workaround was actually in response to @JAgho. The fix to not pass them forward in line 55 is certainly not correct.

JAgho commented 1 year ago

I was reluctant to pass kwargs to everything as I wasn't sure whether the old issue with kwargs triggering dynamic dispatch was still a concern. I tested the ND-TotalVariation branch with MRIReco and it worked without a hitch, performance seemed comparable.

Ultimately looks like a success to me

JakobAsslaender commented 1 year ago

Ha, fair enough, @tknopp, that was my bad! I sketched a version w/o the generic kwargs in PR #51. Can you check if this works?