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

Adds WaveletOperator, WaveletNorm #1615

Merged
merged 51 commits into from
Apr 26, 2024
Merged

Adds WaveletOperator, WaveletNorm #1615

merged 51 commits into from
Apr 26, 2024

Conversation

paskino
Copy link
Contributor

@paskino paskino commented Dec 7, 2023

Describe your changes

WIP for wavelet operator and regularisation.
Joint work with @tommiheik

TODO:

  • check 1D
  • See if we can use OperatorCompositionFunction instead of WaveletNorm by adding proximal
  • check odd shape in wavelet input
  • possibly fix Proximal operator composition with an operator #1561 so that OperatorCompositionFunction can be used straight. Requires some way of assessing if an Operator is orthogonal

Describe any testing you have performed

Please add any demo scripts to CIL-Demos/misc/

Demos added https://github.com/TomographicImaging/CIL-Demos/blob/main/misc/xct_spheres.py

Unit tests to develop:

  • geometry test that we get right range and domain
  • correlation check if wavelet acts on the right dimension
  • known signal, apply direct and adjoint should lead back to the original signal (+- some error) (even/odd size signal)
  • odd/even signal size
  • check that we get the right wavelet for how we configure the operator.

What to do with:

  • complex valued data.
  • non-orthogonal wavelet. Currently everything should be correct for orthogonal wavelets.

Link relevant issues

Closes #1098

Checklist when you are ready to request a review

  • I have performed a self-review of my code
  • I have added docstrings in line with the guidance in the developer guide
  • I have implemented unit tests that cover any new or modified functionality
  • CHANGELOG.md has been updated with any functionality change
  • Request review from all relevant developers
  • Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License.
  • I confirm that the contribution does not violate any intellectual property rights of third parties

@paskino paskino changed the title Added WaveletOperator, WeightedL21, WaveletNorm Added WaveletOperator, WeightedL1Norm, WaveletNorm Dec 7, 2023
@paskino paskino mentioned this pull request Dec 8, 2023
@jakobsj
Copy link
Contributor

jakobsj commented Dec 11, 2023

@paskino This PR has reviews requested but it is in draft mode, mentions WIP, and all todo and checklist boxes not checked. Please can you let me know if/when it is ready for me to review.

@paskino paskino changed the title Added WaveletOperator, WeightedL1Norm, WaveletNorm Adds WaveletOperator, WaveletNorm Dec 15, 2023
Edoardo Pasca and others added 3 commits December 16, 2023 10:15
#1627)

* Fixes for 1D and odd sized geometry
* Better way to slice to odd sized arrays
@paskino
Copy link
Contributor Author

paskino commented Dec 18, 2023

From #1627, some suggestions for unit tests of WaveletOperator

The WaveletOperator should have a proper tests built at some point because there are so many things that can go wrong. For example

  • Check that the dimensions of input and output are correct
  • Check that odd sized arrays behave correctly
  • Check that decomposing different axes works correctly

* Fixed missing wavelet adjoint in the proximal
…nge_geometry` size (#1638)

* Fixes for 1D and odd sized geometry

* Forgot .length property from VectorGeometry

* Better way to slice to odd sized arrays

* Fixed missing wavelet adjoint in the proximal

* Correlation, padding mode and small fixes

- Added string-based `correlation` keywords for defining decomposition `axes`. Specifying `axes` still takes priority though. This is similar behaviour as `GradientOperator`
- Added string-based convolution boundary condition `bnd_cond` to set the wavelet extension mode. This is similar behaviour as `GradientOperator`
- Removed special case for `VectorGeometry`, the user should give the geometry as with 2D and 3D. There was no reason to have this behaviour.

* Checks for range geometry

The size of `range_geometry` is now checked upon initialization to make sure its size matches the wavelet coefficient array size.
@paskino
Copy link
Contributor Author

paskino commented Jan 26, 2024

Unit tests to develop:

  • geometry test that we get right range and domain
  • correlation check if wavelet acts on the right dimension
  • known signal, apply direct and adjoint should lead back to the original signal (+- some error) (even/odd size signal)
  • odd/even signal size
  • check that we get the right wavelet for how we configure the operator.

What to do with:

  • complex valued data.
  • non-orthogonal wavelet. Currently everything should be correct for orthogonal wavelets.

…WaveletNorm (#1665)

Wavelet-related tests and corresponding fixes to WaveletOperator and WaveletNorm
@MargaretDuff
Copy link
Member

Work from @tommheik

Describe your changes

  • WaveletOperator
    • Fixed some geometry checks
    • Correlation keywords are no longer case specific
    • Computes the norm for non-orthogonal wavelets as well (although this will be incorrect due to incorrect adjoint)
  • WaveletNorm
  • test_wavelets.py
    • Created a starting point for wavelet related tests, probably does not work in CIL straight away

Describe any testing you have performed

  • Tests for variety of domain and range geometry sizes
    • With different correlations (some 1D geometry + correlation tests missing but low priority)
    • With different axes
    • Odd and even sizes
    • Exhausting checks for different amount of padding when using different wavelets
  • Test for perfect reconstruction
    • Exhausting checks for different wavelets
    • Test some known signals
    • Test for some well known behaviour with different bnd_cond
  • Tests for adjoint (with bnd_cond='periodization' since it gives the most faithful adjoint
    • Exhausting checks for different orthogonal wavelets (non-orthogonal wavelets skipped for now)
    • Exhausting checks for non-orthogonal wavelets (skipped since adjoint is not properly implemented)
  • WaveletNorm value
  • Proximal values for different tau
    • Extra checks for silly sign errors/bugs
  • Convex conjugate for different inputs

TODO

  • Implement non-orthogonal wavelet adjoint (I have an idea for this, should be relatively easy). For example PDHG should then work with those wavelets too
  • Anything complex valued
  • More testing?

Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Looks like my comments have been addressed and I only now mention two places where I think possibly "positive" should be replaced by "non-negative" if weights can be zero. I hope @tommheik can/has do/ne a final check the maths. Thanks all!

Copy link
Contributor

@tommheik tommheik left a comment

Choose a reason for hiding this comment

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

I tried going through everything with a very nitpicking attitude and found some places for minor improvements or changes. Mainly in the documentation.

Luckily the code seems to be mostly fine.

The tests are maybe bit all over the place. Especially test_functions.py is really difficult to read and maybe in the future it should be split into smaller parts or somehow reorganized...

@MargaretDuff MargaretDuff requested a review from tommheik April 23, 2024 09:08
@MargaretDuff MargaretDuff requested a review from tommheik April 23, 2024 13:16
Copy link
Contributor

@tommheik tommheik left a comment

Choose a reason for hiding this comment

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

Thank you @MargaretDuff for your great efforts! Everything seems to be in order.

Apparently I lack the permissions to directly approve this but I assume you are able to merge anyways.

Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

Approving on behalf of Tommi!

@MargaretDuff MargaretDuff added Merge pending jenkins Once jenkins is happy with can be merged and removed Work In Progress Waiting for review labels Apr 24, 2024
@MargaretDuff
Copy link
Member

Jenkins is happy
image

Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
@MargaretDuff
Copy link
Member

And again with version 24.0.0
image

@MargaretDuff MargaretDuff merged commit 2f92286 into master Apr 26, 2024
8 checks passed
@MargaretDuff MargaretDuff deleted the wavelet_regularisation branch April 26, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge pending jenkins Once jenkins is happy with can be merged
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Proximal operator composition with an operator Wavelet operator
4 participants