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

Adjoint operator #1645

Merged

Conversation

epapoutsellis
Copy link
Contributor

@epapoutsellis epapoutsellis commented Jan 10, 2024

Describe your changes

Added a new adjoint operator in CIL and fixed some errors with the complex matrix operator.

Describe any testing you have performed

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

Link relevant issues

Closes #1643

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 self-requested a review January 11, 2024 08:49
@MargaretDuff
Copy link
Member

This is currently on hold due to the issues with the complex matrix operator in #1646 and #1647

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.

Thanks @epapoutsellis for this work! Just a few minor comments but looks like a good addition and a good fix

@MargaretDuff
Copy link
Member

Also, closes #1646 closes #1647

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.

A good addition of the Adjoint operator - requested by @jakobsj

Also a fix to our matrix operator adjoint

Also a fix to allocate to work with complex datatypes

Copy link
Contributor

@hrobarts hrobarts left a comment

Choose a reason for hiding this comment

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

This is looking good to me! I just had a couple of questions

Wrappers/Python/test/test_DataContainer.py Show resolved Hide resolved
Wrappers/Python/cil/framework/framework.py Show resolved Hide resolved
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, just some tiny editorial comments. No need to see the PR again after fixing.

Wrappers/Python/cil/optimisation/operators/Operator.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/operators/Operator.py Outdated Show resolved Hide resolved
Wrappers/Python/test/test_AdjointOperator.py Outdated Show resolved Hide resolved
@MargaretDuff
Copy link
Member

Jenkins is happy
image

@MargaretDuff MargaretDuff enabled auto-merge (squash) March 5, 2024 17:03
@MargaretDuff MargaretDuff merged commit 9c054f9 into TomographicImaging:master Mar 5, 2024
3 checks passed
casperdcl added a commit to manchester-jhellier/CIL that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

PowerMethod test with complex MatrixOperator Complex MatrixOperator Adjoint Operator
4 participants