Skip to content

Make ReconstructionProperties into a Flag enum to support same reconstructor providing many properties #2292

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

Open
wants to merge 485 commits into
base: main
Choose a base branch
from

Conversation

Tobychev
Copy link
Contributor

A first start at addressing the need in #2291 of a reconstructor indicating it reconstructs many properties.

I changed ReconstructionProperties into a enum of Flag type, meaning a reconstructor can say it provides both geometry and particle type by setting
self.property = ReconstructionProperty.ENERGY | ReconstructionProperty.PARTICLE_TYPE

It is also possible to pick out a list of properties a reconstructor supports given a flag combination in self.property using this (slightly unreadable) list comprehension

        properties = [
            self.property & itm
            for itm in self.supported
            if self.property & itm in ReconstructionProperty
        ]

@Tobychev Tobychev requested a review from maxnoe April 13, 2023 12:26
@maxnoe
Copy link
Member

maxnoe commented Nov 23, 2023

Do we really want a FlagEnum that is using binary operations to store the values instead of just a tuple of the enum like we e.g. do for DataLevel?

What is the advantage of using FlagEnum?

@Tobychev
Copy link
Contributor Author

I didn't think of using a tuple of enums, compared with that case I'm not sure there is any benefit. I can change it.

LukasBeiske and others added 26 commits April 10, 2024 18:48
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4 to 5.
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@v4...v5)

---
updated-dependencies:
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 3 to 4.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@v3...v4)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [JamesIves/github-pages-deploy-action](https://github.com/jamesives/github-pages-deploy-action) from 3.7.1 to 4.5.0.
- [Release notes](https://github.com/jamesives/github-pages-deploy-action/releases)
- [Commits](JamesIves/github-pages-deploy-action@3.7.1...v4.5.0)

---
updated-dependencies:
- dependency-name: JamesIves/github-pages-deploy-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/cache](https://github.com/actions/cache) from 3 to 4.
- [Release notes](https://github.com/actions/cache/releases)
- [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md)
- [Commits](actions/cache@v3...v4)

---
updated-dependencies:
- dependency-name: actions/cache
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Tobychev
Copy link
Contributor Author

@maxnoe You can't properly do it with an tuple, because the StereoCombiner takes the properties via traitlets and traitlets only support type checking fixed length tuples. But traitlets can typecheck variable size sets, so I used that instead.

However, I realised that the assumption that a reconstructor only does one thing seems to be pretty deeply encoded in the predict_table functions via assumptions about the prefix used to name columns, and I don't have a good idea for how to resolve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.