-
Notifications
You must be signed in to change notification settings - Fork 4
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
Quantity basics #2
Conversation
406df6c
to
c89827e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! some quick comments.
quantity/core.py
Outdated
|
||
@property | ||
def __array_namespace__(self): | ||
# TODO: make our own? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think eventually, but not urgently.
574081f
to
3eccc99
Compare
@nstarman, @adrn, @jeffjennings - here's the start of I have a separate branch that implements ufuncs, but want to clean up the tests for that before pushing it. |
quantity/core.py
Outdated
|
||
@dataclass(frozen=True, eq=False) | ||
class Quantity: | ||
value: Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "stuff that implements the Array API".
Unfortunately the Array API doesn't vendor a Protocol themself, but based on the docs the minimal protocol would be
class SupportsArrayAPI(Protocol):
def __array_namespace__(self, *, api_version: str | None = None) -> Any: ...
value: Any | |
value: SupportsArrayAPI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that apart from numpy, no-one actually defines __array_namespace__
(well, at least dask
and jax
do not). Also, I do want to add support for python numbers. Overall, it seems easier to just punt on this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jax should define __array_namespace__
(link.
Hopefully dask will soon as well.
IMO supporting python numbers should be handled by a wrapper class that provides the Array API.
import math
class PythonNumber:
def __init__(self, value): ...
def __array_namespace__(self, ...): return math
If we stick to the Array API it will massively reduce compatibility logic in every function..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right, JAX actually does define it already.
For python numbers, I was thinking along similar lines. That may be where your __value_namespace__
can come in handy. Although perhaps wrapping array_api_compat.array_namespace
may be better.
Anyway, how about we do a typing PR after this one? I do think it is a good idea to start with this as well-typed as possible, but fear I'll need more than a little help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although perhaps wrapping array_api_compat.array_namespace may be better.
Agreed. If we can avoid __value_namespace__
entirely, that would be nice.
a0b50c5
to
e274b6e
Compare
return exp.real if exp.imag == 0 else exp | ||
|
||
|
||
@dataclass(frozen=True, eq=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the major annoyances of dataclasses
is that they chose not to implement the converter
argument to the fields
. https://peps.python.org/pep-0712/. I think this was an atrocious decision that makes everyone's lives more difficult. For us, this means it's much harder to help users when value
is not an Array API-compatible object, like converting a float
to a PythonNumber[float]
-wrapped value.
Thankfully there is a well-defined way around this - the dataclass_transform
decorator. I'd be happy to contribute this in a followup PR. Essentially we produce our own dataclass
decorator which calls python's built-in dataclass
but provides its own __init__
.
def __init__(self, *args, **kwargs):
fs = fields(self)
# Iterate through the fields applying the Field.metadata["converter"] then setting the attribute.
object.__setattr__(self, f.name, value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a plan!
efbda54
to
a0d6d8e
Compare
@nstarman - sorry for the many pushes; clearly too late to get things straight in one go. But now really ready for review (already a lot better given your earlier comments!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks, and suggestions for followup.
It's great to see how much the Array API and dataclasses can simplify our code structure.
|
||
|
||
def get_value_and_unit(arg, default_unit=None): | ||
# HACK: interoperability with astropy Quantity. Have protocol? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Something like
@runtime_checkable
class QuantityAPI(Protocol):
value: Array
unit: Unit
def to(self, ...): ...
Then this function becomes
def get_value_and_unit(arg, default_unit=None):
return (arg.value, arg.unit) if isinstance(arg, QuantityAPI) else (arg, default_unit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to actually raise an error on having a .unit
attribute and not .value
(e.g., how do we deal with Column
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.s. There's part of me that still would like to avoid Quantity
having anything to do with unit conversion - that can be up to the unit. E.g., q.to_value(unit)
could become q // unit
or q >> unit
or so (somewhat partial to //
myself, since it is close - q1 // q2
already returns a dimensionless quantity, so all one really needs is to avoid acting on the "make integer" part).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in not have .to()
and .to_value()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK to postpone this until we have a clearer idea what exactly the Protocol
will be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Should we open an Issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3
quantity/core.py
Outdated
|
||
# Operators (skipping ones that make no sense, like __and__); | ||
# __pow__ and __rpow__ need special treatment and are defined below. | ||
__add__, __radd__, __iadd__ = _make2arg_ops("__add__") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As followup, we should get mypy
set up to do static testing to make sure it's happy with this magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, maybe typing the lot should be next so that we can start with a smaller base to test - then the (u)functions after that!
a0d6d8e
to
80b97fc
Compare
@nstarman - I updated the minimum versions. Shall we go ahead and get this in, so we can move to the next step (typing?). |
Yes! Are you ok to do squash merge commits, or do you want to clean up the commit history? |
I think this is fairly clean - or do you want me to go back and add version numbers when I introduce the dependencies? |
LGTM. |
🎉 |
Support for
numpy
,array-api-strict
,dask
, andjax
, with basic tests for all, and ufuncs fornumpy
only. Pushing now in part to check whether CI works properly.EDIT: I pushed a version that does not yet include numpy ufuncs, but for which the basics are now tested more thoroughly.