CIL icon indicating copy to clipboard operation
CIL copied to clipboard

BlockOperator direct and adjoint methods: can pass out as a DataContainer instead of a (1,1) BlockDataContainer where geometry permits

Open MargaretDuff opened this issue 1 year ago • 2 comments

Description

Linked to #1863

Changes

Testing you performed

Please add any demo scripts to https://github.com/TomographicImaging/CIL-Demos/tree/main/misc

Related issues/links

Checklist

  • [x] I have performed a self-review of my code
  • [x] I have added docstrings in line with the guidance in the developer guide
  • [ ] I have updated the relevant documentation
  • [x] I have implemented unit tests that cover any new or modified functionality
  • [ ] CHANGELOG.md has been updated with any functionality change
  • [x] Request review from all relevant developers
  • [x] 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 avatar Sep 03 '24 16:09 MargaretDuff

So in #1802 we changed BlockOperator direct (and adjoint) methods to return a DataContainer in the case it would previously return a (1,1) BlockDataContainer. We also changed the range (and domain) to output the required geometry in this case.

In this PR we allow BlockOperator direct (and adjoint) methods to accept both a (1,1) BlockDataContainer or a DataContainer to out where that fits with the range (and domain) geometry. They already accept both (1,1) BlockDataContainer or a DataContainer as an input argument where the domain (and range) geometry allows.

The question is: should a BlockOperator always return a BlockDataContainer or are (1,1) BlockDataContainers an unnecessary wrapper and they should be DataContainers?

In the first case, we should write an as_array() method for (1,1) BlockDataContainers to fix the original CIL-SIRF issue https://github.com/TomographicImaging/CIL/issues/1455 and probably leave this fix in. In the second case, we leave #1802 as it is and implement this fix.

MargaretDuff avatar Sep 04 '24 10:09 MargaretDuff

After discussion with Edo and Gemma we decided that this should go in but that we would still further investigate #1863

MargaretDuff avatar Oct 07 '24 12:10 MargaretDuff

@gfardell - do you want to check the adjoint code and approve if you are happy?

MargaretDuff avatar Nov 19 '24 16:11 MargaretDuff

We need to decide the default. If no out is provided to direct(adjoint) but it should return a 1 by 1 block data container, should this be a data container -to match the range (domain) - or be a block data container?

MargaretDuff avatar Nov 19 '24 16:11 MargaretDuff