-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
src/dimension_api/__init__.py
Outdated
|
||
@runtime_checkable | ||
class Dimension(Protocol): | ||
def __eq__(self, other: Self, /) -> bool: ... |
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.
__eq__
is redundant here, because it's already defined in object
:
https://github.com/python/typeshed/blob/aac4394eb29d86797628b57d6f01f5e17f5ff83f/stdlib/builtins.pyi#L117
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'm not sure: requiring __eq__
to work with other: Self
seems like a more restrictive requirement (though, I'm often unsure how to interpret object
used as a type annotation). Unresolving this thread 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.
I guess object
in an annotation within the object
class is effectively doing the same as what Self
is doing?
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.
It might, but in any other context I take it as meaning "this variable cannot be assume to conform to any interface", which is radically different.
I suspect maybe @nstarman knows whether they are truly equivalent here.
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.
Comparison dunder methods are complicated. They are meant to work with all input, returning NotImplemented
if they don't know how to perform the comparison, and bool
if they do (obv numpy extends this to Array[bool]).
What we want is approximately
@overload
def __eq__(self, other: object) -> NotImplemented: ...
@overload
def __eq__(self, other: Self) -> bool: ...
def __eq__(self, other: object) -> NotImplemented | bool
A problem in Python is that type checkers really don't like if you annotate the return type as NotImplemented | bool
!
@jorenham, I'm looking at optype
for how to type this precisely, but AFAIK it should just be
def __eq__(self, other: object) -> bool
Which is the builtin, but here and elsewhere we should add some docs!
def __eq__(self, other: object) -> bool
"""Equality comparison.
Comparison between two Dimension objects (of the same type) must return a `bool`.
Comparison with other objects is outside the scope of this API.
"""
Co-authored-by: Joren Hammudoglu <[email protected]>
It looks good, the major issue is that |
|
Should dimensions also be orderable, i.e. implement |
Or perhaps something like |
Looking at what the existing implementations have:
Does |
|
Sorry, just reacting to a ping and short of time, so really just a quaestion: what is a |
The former, I think. |
I'm pretty sure we're implementing this (from wikipedia):
where their |
OK, I see. Seems a bit secondary -- at least in astropy we've had physical types for a while now but I don't know that I've seen anybody actually using it. Not sure that it needs a new class/protocol. Wouldn't it be fine if we simply required that a unit class has the ability to produce a composite unit that uses only a given set of bases? Anyway, maybe best to try to nail down the unit API first? |
I think it is worth adding a docstring for the class as it's not completely obvious what its purpose is.
Yes, it is these base dimensions. Although there may be more or fewer dimensions; some libraries use angle or information as a dimension, someone may wish to use currency. astropy's physical type is more similar to quantitykind in other libraries/databases. However quantitykind is a property of a Quantity and is stricter, preventing a torque quantity from being added to an energy. The purpose of this class is to allow a user to check the dimensions of a Unit or Quantity in a consistent manner. Without this the dimension property could return anything, so not relied on when writing code to support multiple libraries.
in pint that would be compared to using dimensionality: note that the base units could be any unit system which is why calls to
|
I'm a big fan of this idea and would like to hear more about how this would be implemented API-wise. How does it fit into UAPI vs DAPI? Is it an intermediate level? |
OK, so astropy's equivalent is Note that at least for us, |
it's similar to units, and can also be heirachial, see https://github.com/qudt/qudt-public-repo/wiki/Advanced-User-Guide#4-computing-applicable-units-for-a-quantitykind I made a start at an implementation hgrecco/pint#1967 the only released implementation I have seen is described in e: there's many more quantitykinds than units so the plan in my head is for pint to move to a new unit file format, then parse the QUDT ontology database using the new file format |
I think this API is ready! |
@mhvk another use of dimensions is for dimensional analysis e.g solving Buckingham Pi type problems. Physics-wise this is important for analytics (eg Sympy's quantities), symbolic regression, etc, and then just expedited quantity checking more broadly. |
@andrewgsavage - thanks for those links - the |
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, if int
for the power is fine typing wise in leaving room for fractions in implementing libraries, I'm happy with this.
@neutrinoceros how does this look to you? |
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've removed is_dimensionless
from this PR and opened https://github.com/quantity-dev/dimension-api/issues/2.
I think there is nothing to vote on for this PR now. I plan to merge it soon if there are no objections.
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 agree nothing feels controversial in this PR.
Thoughts:
- Don't love the Pixi stuff. I have the sense more people use uv. But we can easily support both without conflict so 🤷 SGTM. Which we used is a discussion for later when we set up CI; and even then doesn't impact whether there's config for both.
- dynamic versioning. For future PR.
- this can't be released without #2 being resolved, but that's irrelevant to whether this should be merged.
Thanks @lucascolley for this PR!
thanks all, in it goes |
@nstarman @jorenham @andrewgsavage does this look about right?