Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CCPi Regularisation toolkit plugin: bugfix, docstrings, unittests, remove unused functions #971

Merged
merged 14 commits into from
Oct 21, 2021

Conversation

paskino
Copy link
Contributor

@paskino paskino commented Sep 20, 2021

closes #931

@paskino
Copy link
Contributor Author

paskino commented Sep 20, 2021

Please @epapoutsellis and/or @jakobsj can you advise on what to do with LLT_ROF? There are 2 regularisation parameters, and I currently have multiplied both of them.

@epapoutsellis
Copy link
Contributor

I think we delayed a lot adding tests for the regularisation toolkit. The only option easy and fast is via cvxpy.

@paskino
Copy link
Contributor Author

paskino commented Sep 27, 2021

I don't know. Tests should be implemented in the CCPi Regularisation package. Some are already implemented, though they test each other or the C/OpenMP vs the CUDA code.

But the tests you want to implement would be good to have anyway. Open an issue about it.

@paskino
Copy link
Contributor Author

paskino commented Oct 2, 2021

An issue is already open for TNV: TomographicImaging/CCPi-Regularisation-Toolkit#146

@paskino
Copy link
Contributor Author

paskino commented Oct 4, 2021

Actually we only use FGP_TV, FGP_dTV, TGV and TNV. So we should scrap all the other plugins and add appropriate unittests for these in the CCPi-Regularisation toolkit

Also check if there are unittests for the plugin functionality.

@paskino
Copy link
Contributor Author

paskino commented Oct 15, 2021

OK, I removed all the regularisng functions we do not use. I added unit tests to compare our plugin via proximal and the direct call of the regulariser.

Could you please confirm the defaults I chose?

@paskino paskino requested a review from gfardell October 15, 2021 13:02
@paskino
Copy link
Contributor Author

paskino commented Oct 15, 2021

I also added __rmul__ to this PR, so that constructs like

f = alpha * FGP_TV()

return a FGP_TV function with an internal regularisation parameter multiplied by alpha rather than a ScaledFunction

@paskino
Copy link
Contributor Author

paskino commented Oct 15, 2021

Jenkin's happy.

Copy link
Member

@gfardell gfardell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need docstrings for the 4 we're wrapping, but that's not really the fault of this PR!

@epapoutsellis
Copy link
Contributor

epapoutsellis commented Oct 18, 2021

The default ratio for the TGV parameters should be alpha0\alpha1 \in [1,2]

https://github.com/vais-ral/CCPi-Regularisation-Toolkit/blob/413c6001003c6f1272aeb43152654baaf0c8a423/demos/demo_cpu_regularisers.py#L312

@paskino
Copy link
Contributor Author

paskino commented Oct 19, 2021

Notice that I removed the printing parameter from the calls as useless. I changed to the max_iterations member in each function as that we a more appropriate name.

@paskino paskino changed the title use tau parameter in all regulariser functions CCPi Regularisation toolkit plugin: bugfix, docstrings, unittests, remove unused functions Oct 19, 2021
@paskino
Copy link
Contributor Author

paskino commented Oct 21, 2021

Jenkin's happy

@paskino paskino requested a review from epapoutsellis October 21, 2021 11:28
@paskino paskino requested a review from epapoutsellis October 21, 2021 12:25
Copy link
Contributor

@epapoutsellis epapoutsellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM happy to merge

@paskino paskino merged commit cf87ba9 into TomographicImaging:master Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regularisers plugin: tau is not always used in the proximal methods
3 participants