✨feat: dataclasses with converters

[nstarman]

@mhvk, as promised 😎.
IMO this should be spun off into a separate utility package, so it can be used, e.g. for the coordinates APE. CC @jeffjennings & @adrn.

[mhvk]

Looks nice. But I'm not quite sure whether we want to necessarily lock ourselves in too much: e.g., for python scalars, we don't really want to do np.asarray, right? Though I guess we can relax that later, and then having the ability to wrap the scalar in a light class that provides .shape() etc would be good.

Similarly, in principle one would like to worry as little as possible about the unit class (but again, can relax that later).

Probably also premature, but does this get executed only on first initialization, or also on replace? I.e., is there a performance penalty for every operation?

[nstarman]

But I'm not quite sure whether we want to necessarily lock ourselves in too much: e.g., for python scalars, we don't really want to do np.asarray, right? Though I guess we can relax that later, and then having the ability to wrap the scalar in a light class that provides .shape() etc would be good.

Agreed. We probably want to do PythonScalar(float) rather than ndarray.

Similarly, in principle one would like to worry as little as possible about the unit class (but again, can relax that later).

Yes, the converter for the units field allows us to delegate determining what's a viable input to the unit constructor, rather than Quantity.

Probably also premature, but does this get executed only on first initialization, or also on replace? I.e., is there a performance penalty for every operation?

Yes there is. We can provide a past-path around this. e.g.

    @functools.wraps(dcls.__init__)  # give it the same signature
    def __init__(self, *args: Any, _fastpath: bool = False, **kwargs: Any) -> None:
        if _fastpath:
            return self.__init__.__wrapped__(self, *args, **kwargs)
        ...

Then the only performance penalty is in calling the wrapper init. I think we can mitigate even that function call overhead using mypyc to turn this decorator into C code.

[nstarman]

Done. I enabled fast-pathing.

[nstarman]

@mhvk how does this look now?

[nstarman]

In future, to make this completely unit-type agnostic, we would set the type to be a runtime-checkable protocol and the converter would be something like converter=lambda x: x if isinstance(x, UnitAPI) else config.get("units_backend")(x), where the user can configure a preferred units package. Or, if we keep this more closely tied to Astropy, we would have converter=lambda x: x if isinstance(x, UnitAPI) else u.Unit(x)

[mhvk]

I think this may be the way to go but worry that it is a bit too soon. Partially because I still think this is a bit too fast in turning python floats etc into numpy arrays -- after all, they promote differently, and also they are not annoyingly eager in converting everything else to ndarray.

But also because it is not currently strict enough: e.g., what do we do with Quantity(q, unit)? (with q either our own quantity or astropy's, or even an astropy Column) Do we bail? Do we convert, like astropy? If so, one needs access to the unit, and defining __init__ would be more appropriate.

My overall sense would be to postpone deciding on this, and first try to get things working assuming input is sane (though of course that should not stop us trying to reason things through if it isn't!).

[nstarman]

Partially because I still think this is a bit too fast in turning python floats etc into numpy arrays -- after all, they promote differently, and also they are not annoyingly eager in converting everything else to ndarray.

Agreed about the promotion. I do think that strictly enforcing that Quantity.value is Array API will make maintenance significantly easier. Only the constructor needs to consider non Array API inputs. Everything after that is just Array API . If we merge this now I can start working on a PythonArrayScalar class that implements the API and is a thin wrapper around a Python Number. Then it will be trivial to change the value converter from x if isinstance(x, ArrayAPI) else np.array(x) to

if isinstance(x, ArrayAPI):
    return x
elif isinstance(Number):
    return PythonArrayScalar(x)
else:
    return np.array(x)

But also because it is not currently strict enough: e.g., what do we do with Quantity(q, unit)? (with q either our own quantity or astropy's, or even an astropy Column) Do we bail? Do we convert, like astropy? If so, one needs access to the unit, and defining init would be more appropriate.

I think we do what numpy does. np.ndarray can be used to construct arrays, but it expects good input and doesn't do much user handholding. If you want a more general constructor, then use np.array(...).
Let's do similarly for Quantity and quantity():

• Quantity accepts 2 arguments, something that can be directly converted to the value, e.g. a non-quantity ArrayAPI object or Python scalar and something that can be interpreted as the unit using a unit constructor.
• quantity() can do a lot more interpretation. quantity(Quantity) -> Self, quantity("1m") -> Quantity(PythonArrayScalar(1), Unit("m")), quantity({"value": [1], "unit": "m"}) -> Quantity(np.array([1]), Unit("m"))

Therefore I would suggest using __post_init__ or the converter= to do basic checking, e.g.

@dataclass
class Quantity
    def __post_init__(self) -> None:
        assert not isinstance(self.value, Quantity)

or

if isinstance(x, Quantity):
    raise ValueError("use quantity(...)")
elif isinstance(x, ArrayAPI):
    return x
elif isinstance(Number):
    return PythonArrayScalar(x)
else:
    return np.array(x)

IMO the lesson to learn from numpy ndarray is that we want a generalized constructor for Quantity, not that Quantity.__init__ needs to be that constructor.