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

Refactor framework #1692

Merged

Conversation

manchester-jhellier
Copy link
Contributor

@manchester-jhellier manchester-jhellier commented Feb 8, 2024

  1. DataOrder is densely coupled to things in framework.py (especially DataContainer). To alleviate this, {ImageGeometry,AcquisitionGeometry}.{CHANNEL,VERTICAL,...} is replaced with with labels.Enum (fixes DataOrder is a problem and should be destroyed #1691)
  2. break framework.py into smaller pieces (fixes Break framework.py into smaller pieces #1686)

@casperdcl checklist

  • rebase/merge
  • rework labels
    • use StrEnum
    • combine AcquisitionTypes & AcquisitionDimensions into AcquisitionType(enum.Flag)
    • better OOP for {get,check}_order_for_engine()
    • discuss/debate naming (e.g. FillTypes -> FillType etc.)

tests

  • CIL CI
  • SIRF CI?

review checklist

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

… with calls to a TypedDict. Remaining bug with Partitioner import in 2 test cases.
@manchester-jhellier manchester-jhellier changed the title Prepare to kill DataOrder Refactor framework Feb 10, 2024
@manchester-jhellier
Copy link
Contributor Author

manchester-jhellier commented Feb 10, 2024

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

@manchester-jhellier
Copy link
Contributor Author

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).

Wrappers/Python/cil/framework/labels.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/framework/labels.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/framework/labels.py Outdated Show resolved Hide resolved
@hrobarts
Copy link
Contributor

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 TomographicImaging/CIL-Demos#155 .
Happy to approve :)

@MargaretDuff
Copy link
Member

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

@gfardell gfardell self-requested a review August 21, 2024 16:10
@gfardell
Copy link
Member

image

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 casperdcl merged commit f708a1f into TomographicImaging:master Aug 22, 2024
8 checks passed
@casperdcl
Copy link
Member

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

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

DataOrder is a problem and should be destroyed Break framework.py into smaller pieces
7 participants