Refactor framework

[manchester-jhellier]

Describe your changes

DataOrder densely coupled to other entities in framework.py (in particular DataContainer which we seek to isolate in the grander scheme of this hackathon's refactoring). To alleviate this we replace class members on ImageGeometry and AcquisitionGeometry with references to a TypedDict in a file. We've then gone onto pull many things out of framework.py and rename it.

Describe any testing you have performed

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

Passes CIL test suite, including SIRF things.

Link relevant issues

• closes #1686
• closes #1691

Checklist when you are ready to request a review

• [x] I have performed a self-review of my code
• [x] I have added docstrings in line with the guidance in the developer guide
• [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'
• [ ] check notebooks

Contribution Notes

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

• [x] 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.
• [x] I confirm that the contribution does not violate any intellectual property rights of third parties

[manchester-jhellier]

Added a whole load of new commits to break framework.py into smaller files. Largest file is now only ~1900 lines long!

[manchester-jhellier]

SIRF tests run using Docker, and those have been amended so they pass (turned out there was a lot less stuff being imported from CIL than I'd thought).

[casperdcl]

@manchester-jhellier I just fixed the merge conflicts :)

[manchester-jhellier]

@manchester-jhellier I just fixed the merge conflicts :)

Oh cool, so you don't need me to do that? I hadn't gotten around to it, sorry about that. Is there stuff you do need me to do? It seemed like there was a dispute about licensing, are you guys happy with that now? J

[casperdcl]

just one error left:

======================================================================
FAIL: test_VectorGeometry_allocate_complex (test_DataContainer.TestDataContainer.test_VectorGeometry_allocate_complex)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "CIL/Wrappers/Python/test/test_DataContainer.py", line 729, in test_VectorGeometry_allocate_complex
    self.assertNotEqual(numpy.sum(data.array).imag, 0)
AssertionError: 0.0 == 0

[paskino]

Error seems of type?

[casperdcl]

According to https://github.com/manchester-jhellier/CIL/blob/daae9d2eda5a6d627e2cf08cf10ff2121b4c444a/Wrappers/Python/cil/framework/data_container.py#L1527-L1541 it seems RANDOM does both real & imag, while RANDOM_INT only does real (so imag == 0).

[casperdcl]

Looks like is was missing: https://github.com/TomographicImaging/CIL/blob/eb254e0bae77e193f05acc03b6e94cd1c189e4ab/Wrappers/Python/cil/framework/framework.py#L4251-L4254

[MargaretDuff]

Checked on the CIL demo notebooks and hasn't broken any backward compatibility there

• [ ] binder
• [x] colab
• [x] demos/1_Introduction
• [ ] demos/2_Iterative
• [x] demos/3_Multichannel @casperdcl
• [ ] demos/appendix.ipynb
• [ ] misc

moved to description

[gfardell]

I don't really like how things are implemented in 'label.py'. I can see what it's trying to achieve and agree something is needed but I think it needs some thought. As it is, I can see it's only internal api changes so maybe we could revisit it, but we might as well come up with something that benefits the user too. At the very least dividing the labels in to types (dimension labels, datatypes, geometry types).

I think we should discuss approaches to this before we merge.

[gfardell]

• We need to add Josh and Nick to https://github.com/TomographicImaging/CIL/blob/master/NOTICE.txt.
• We also need to add them as authors to files where they added code (I guess use git to check this). Although very appreciated and useful I don't think refactoring existing code will count as adding copyrighted material - but I'm happy to discuss.
• Update changelog (I added the labels part, but not the refactor)
• Check documentation renders correctly
• Fix my labels EnumMeta hack!

moved to description

[hrobarts]

I checked the documentation and it looks fine. I added the labels separately but can remove them if we don't want them in the user-facing docs. I also re-checked some demo notebooks (Introduction, binder, colab, a couple in misc) and they all seem to be fine apart from the warnings we know about for v 24 https://github.com/TomographicImaging/CIL-Demos/issues/155 . Happy to approve :)

[MargaretDuff]

Have run a few of the iterative (folder 2) demo notebooks and all is fine there

[gfardell]

The documentation is a bit odd. The class name is strange, and the white space between the attributes. I guess it's useful to have the classes in the documentation, but I'm not sure the best way to show the attributes and values people can use/get.

Does anyone have any ideas/examples?

[casperdcl]

Congratulations @manchester-jhellier you are now 4th in the list!