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

BlockOperator: added a function to return a list of norms and the ability to set this list of norms #1513

Merged

Conversation

MargaretDuff
Copy link
Member

@MargaretDuff MargaretDuff commented Oct 2, 2023

Describe your changes

BlockOperator.norms() lists the norms of each individual operator in the block, using the operator functions to either call the cached value or calculate it and cache the value. BlockOperator.set_norms() uses the set_norm function of each individual operator to set the cached value of the norm of the underlying operators.

Relevant unit tests have been added.

Describe any testing you have performed

Link relevant issues

Closes #1510

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

@MargaretDuff MargaretDuff changed the title BlockOperator added a function to return a list of norms and the ability to set_norms BlockOperator: added a function to return a list of norms and the ability to set this list of norms Oct 2, 2023
@MargaretDuff
Copy link
Member Author

P.s. It might be worth thinking about issue #1484 at the same time or instead leaving it as a separate PR?

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 good to me!

Copy link
Contributor

@paskino paskino left a comment

Choose a reason for hiding this comment

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

Apart for one suggestion of a docstring and removal of an old commented piece of code, it looks good to me.

Please do open issues as per comments.
https://github.com/TomographicImaging/CIL/pull/1513/files#r1362075817
https://github.com/TomographicImaging/CIL/pull/1513/files#r1362089405

@MargaretDuff MargaretDuff added the Merge pending jenkins Once jenkins is happy with can be merged label Oct 18, 2023
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.

Just docstring based comments, once they are done I'm happy.

@MargaretDuff MargaretDuff merged commit 72404e6 into TomographicImaging:master Nov 8, 2023
3 checks passed
@MargaretDuff MargaretDuff deleted the blockoperator-norms branch November 8, 2023 10:04
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 Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlockOperator norms
5 participants