Typing issues

[henryiii]

This is a holder for any problems or questions that come up about boost-histogram's static typing support.

[henryiii]

I forgot to add ellipsis support to getitem/setitem, so a valid expression like this:

h[...] = np.ones(10)

will fail a type check. There isn't a PEP 484 way to type check an ellipsis; only "ellipsis" exists as a workaround for now (and it trips up linters, see PyCQA/pyflakes#616). Since we target mypy pretty heavily, and pytype at least also supports this, I'm happy to add the workaround for now, and I've opened a discussion on typing-sig to see if it can get standardized. Possibly as Ellipsis or typing.EllipsisType.

[henryiii]

Note to 1.0.0 users: this is obviously valid, you can just stick a # type: ignore on it for now and use it at runtime.

[alexander-held]

When subclassing Histogram, is the following constructor the correct way to convert a bh.Histogram into a custom NewHistogram?

import boost_histogram as bh
import numpy as np

class NewHistogram(bh.Histogram, family=object()):
    def __init__(self, hist: bh.Histogram) -> None:
        super().__init__(hist, metadata=None)

    @property
    def x(self) -> np.ndarray:
        return self.values()

def f(hist: NewHistogram) -> None:
    ...

h = bh.Histogram(bh.axis.Variable([0, 10]), storage=bh.storage.Weight())
h.fill([1,2,3])
a = NewHistogram(h)
f(a)
a.x

The constructor is now needed to avoid the mypy error

error: No overload variant of "NewHistogram" matches argument type "Histogram"  [call-overload]

Previously (boost-histogram<1.0) I have used a structure similar to the above but without the constructor.

I am unsure in particular about metadata=None, it seems like h.metadata is passed through to a.metadata anyway, and the above executes without metadata=None, but then mypy again fails with call-overload.

Sorry I previously missed this discussion topic for type-related issues when opening #527!

[henryiii]

I have two bugs for the metadata - I forgot to leave it off for regular Histograms (Only CppHistograms are missing metadata, since they are not Python objects), and I forgot to make it optional; for an overload, you can set it to ... to make it optional.

Opening an issue for a clear bug is fine as well. The Discussion keeps it a little more organized, and is only clearly better if someone is not sure that the behavior is a bug. But in 1.0.0, most typing oddities are likely bugs. :)

[henryiii]

I also have an intentional bug - Histogram(hist1, hist2) is invalid at runtime, but passes typing. I don't think I can fix that until this is Python 3.8+ only.

[henryiii]

Fixing this includes this change:

-        # TODO: This might be a bug in MyPy
-        other: H = self.__class__(_hist)  # type: ignore
+        other = self.__class__(_hist)

That's both fantastic, and rather embarrassing....

[henryiii]

So far all of these fixes have removed type: ignore's :)

[alexander-held]

Thanks a lot for the follow up and the fixes! I believe this means I would not need to implement the constructor at all. This is especially helpful since in my actual implementation I also use the other constructor version to create a Histogram from an axis, and I guess that would have required some kind of @typing.overload like in boost-histogram itself.

[henryiii]

Generic histograms actually would work pretty well - MyPy can infer the type of the histogram from the arguments, so Histogram(..., storage=Weight()) should actually be inferred to be Histogram[Weight]. (might be a bit trickier with Hist's ability to avoid writing storage=). The only problem now is I don't know how to provide a "mapping" between Storage and View. I've asked on the typing gitter channel. Basically, this:

from __future__ import annotations

from typing import Generic, NewType, TypeVar

class A: pass
class B: pass
class C: pass

AView = NewType("AView", int)
BView = NewType("BView", int)

S = TypeVar("S", A, B, C)

class MyGeneric(Generic[S]):
    def __init__(self, s: S):
        self.s: S = s

    def storage(self) -> S:
        return self.s

    def value(self) -> AView | BView:
        if isinstance(self.s, A):
            return AView(0)
        else:
            return BView(0)

gen = MyGeneric(A())
value: AView = gen.value()
# Produces:
# : Incompatible types in assignment (expression has type "Union[AView, BView]", variable has type "AView")

I don't know what to write instead of the Union there.

[henryiii]

Got it:

    @overload
    def value(self: "MyGeneric[A]") -> AView: ...
    @overload
    def value(self: "MyGeneric[B]") -> BView: ...
    @overload
    def value(self: "MyGeneric[C]") -> BView: ...

Beat a reply in Gitter. :) So I think I can fix this, and it mostly will "just work" with no user changes.

[henryiii]

It will require some work still, "Histogram" by itself causes complaints (so this might need a biggish version bump, also there should be a way to specify a generic "Histogram"). There are also some other issues, like MyPy not liking setting a default for the thing that picks the type; storage: S = Double() shouldn't error out, I think. (It's also ugly and requires noqa already, you should not call something in a default constructor, but Optional[S] = None would likely not compute the correct S when it was defaulted.) But I think the general idea is doable in the future.

[alexander-held]

Running mypy with boost-histogram in Python 3.7 does not work due to PEP 570 / syntax in function arguments like this:

boost-histogram/src/boost_histogram/_core/accumulators.pyi

Line 22 in 2fef7a0

def __iadd__(self: T, val: float, /) -> T: ...

A workaround is not checking boost-histogram via mypy --follow-imports skip. This means the following would only succeed with a # type: ignore, since otherwise the family kwarg is unknown:

import boost_histogram as bh

class Hist(bh.Histogram, family=object()):
    ...

Would the recommendation be to not run mypy in Python<3.8 at all, or is there another way to approach this?

[henryiii]

It does not work from 3.6 for sure, since NumPy requires 3.7. It's just 3.7 that's problematic.

[henryiii]

Can you try latest the develop branch? Should be fixed in #542

[alexander-held]

I gave this a try, there still seem to be a few places where the slash is used:

boost-histogram/src/boost_histogram/_core/axis/__init__.pyi

Line 42 in 67b96f0

def index(self, value: ArrayLike, /) -> int | np.ndarray: ...

mypy fails consequently in Python 3.7.

[henryiii]

Now, after #545? I've searched for /) this time.

[alexander-held]

Looks good now, thanks!