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

Implement histogram LH5 specification #15

Open
gipert opened this issue Sep 18, 2023 · 5 comments
Open

Implement histogram LH5 specification #15

gipert opened this issue Sep 18, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@gipert
Copy link
Member

gipert commented Sep 18, 2023

Serialization/deserialization of hist.Hist objects to LH5.

Specification: https://legend-exp.github.io/legend-data-format-specs/dev/hdf5/#Histograms

@gipert gipert added the enhancement New feature or request label Sep 18, 2023
@MoritzNeuberger
Copy link
Contributor

So I experimented a bit with a possible implementation. @gipert had the idea to implement a LGDO wrapper for hist.Hist (or more precisely hist.BaseHist [link]) using multiple inheritance. One problem I ran into while playing around is that I get a TypeError mentioning that the metaclasses of LGDO and Hist are not compatible. I'm not an expert in this area, so I don't really know how to fix this. Do you have any idea how to work around this?

@MoritzNeuberger
Copy link
Contributor

MoritzNeuberger commented Jan 18, 2024

The implementation so far looks like this btw:

class Histogram(LGDO,Hist):
    def __init__(self, 
                *in_args: hist.AxisTypes | hist.Storage | str,
                storage: hist.Storage | str | None = None,
                metadata: Any = None,
                data: np.typing.NDArray[Any] | None = None,
                label: str | None = None,
                name: str | None = None,
                attrs: dict[str, Any] = None,) -> None:

        LGDO.__init__(self,attrs)
        hist.Hist.__init__(self,in_args,storage,metadata,data,label,name)

@gipert
Copy link
Member Author

gipert commented Jan 18, 2024

Indeed:

>>> from hist import Hist
>>> from lgdo import LGDO
>>>
>>> class Histogram(LGDO, Hist):
>>>     def __init__(
>>>         self,
>>>         attrs=None,
>>>         **kwargs,
>>>     ):
>>>         super(LGDO).__init__(attrs)
>>>         super(Hist).__init__(**kwargs)

TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

But I'm not an expert, I guess some more research is needed. Also hist ppl seem to use quite advanced python subclassing stuff in basehist.py, we could maybe ask them.

@gipert
Copy link
Member Author

gipert commented May 11, 2024

@ManuelHu
Copy link

ManuelHu commented May 14, 2024

So the problem is really that LGDO has ABC as a metaclass, and Hist has something different. Even

from hist import Hist
from lgdo import LGDO

class Histogram(LGDO, Hist):
    pass

is already throwing.


But

from abc import ABCMeta
from hist import Hist
from lgdo import LGDO

class HistogramMeta(ABCMeta, Hist.__class__):
    pass

class Histogram(LGDO, Hist, metaclass=HistogramMeta):
    pass

is working :-) But I have to look further into this if this actually works...


edit: Further down the line this does not work. I failed to even call the Hist constructor with this wrapping.

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

No branches or pull requests

3 participants