cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
64 stars 268 forks source link

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

Open Tobychev opened 1 year ago

Tobychev commented 1 year ago

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
        ]
maxnoe commented 11 months ago

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 commented 11 months ago

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.

review-notebook-app[bot] commented 7 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Tobychev commented 7 months ago

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