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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 24 commits into from
Nov 8, 2023
Merged

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

merged 24 commits into from
Nov 8, 2023

Conversation

MargaretDuff
Copy link
Member

@MargaretDuff MargaretDuff commented Oct 2, 2023 •
edited
Loading

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?

MargaretDuff and others added 3 commits October 9, 2023 08:52
@MargaretDuff
Comments from Edo fixed
1ca3a2b
@MargaretDuff
Merge branch 'master' into blockoperator-norms
4b541e7 
Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
@MargaretDuff
Fixed tests
5a302c8
jakobsj
jakobsj approved these changes Oct 12, 2023
View reviewed changes
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!

paskino
paskino approved these changes Oct 17, 2023
View reviewed changes
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 mentioned this pull request Oct 18, 2023
Open
@MargaretDuff MargaretDuff requested a review from gfardell October 18, 2023 10:29
@MargaretDuff MargaretDuff added the Merge pending jenkins Once jenkins is happy with can be merged label Oct 18, 2023
@MargaretDuff MargaretDuff mentioned this pull request Oct 26, 2023
Closed
8 tasks
@MargaretDuff MargaretDuff mentioned this pull request Oct 26, 2023
Open
gfardell
gfardell requested changes Nov 6, 2023
View reviewed changes
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.

Wrappers/Python/cil/optimisation/operators/BlockOperator.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/operators/BlockOperator.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/operators/BlockOperator.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/operators/BlockOperator.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/operators/BlockOperator.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/operators/Operator.py Show resolved Hide resolved
@MargaretDuff MargaretDuff requested a review from gfardell November 7, 2023 16:40
@MargaretDuff @gfardell
Update Wrappers/Python/cil/optimisation/operators/Operator.py
3d40876 
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com>
Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
@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
@MargaretDuff MargaretDuff mentioned this pull request Jan 10, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epapoutsellis epapoutsellis epapoutsellis left review comments

@paskino paskino paskino approved these changes

@gfardell gfardell gfardell approved these changes

@jakobsj jakobsj jakobsj approved these changes

Assignees
No one assigned
Labels
Merge pending jenkins Once jenkins is happy with can be merged Waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BlockOperator norms
5 participants
@MargaretDuff @jakobsj @paskino @epapoutsellis @gfardell