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

Quantity should be an abstract base class #821

Open
llucax opened this issue Dec 18, 2023 · 0 comments · May be fixed by #822
Open

Quantity should be an abstract base class #821

llucax opened this issue Dec 18, 2023 · 0 comments · May be fixed by #822
Assignees
Labels
part:actor Affects an actor ot the actors utilities (decorator, etc.) part:core Affects the SDK core components (data structures, etc.) part:data-pipeline Affects the data pipeline scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users
Milestone

Comments

@llucax
Copy link
Contributor

llucax commented Dec 18, 2023

What's needed?

We should not allow to use a plan Quantity without any associated... quantity, as that's just a float.

Proposed solution

Make Quantity a ABC.

Use cases

No response

Alternatives and workarounds

No response

Additional context

The solution is probably much more complex than that, as I believe we allowed to construct "abstract quantities" to overcome complications with the type system, in particular related to Samples, that sometimes need to have a float value and sometimes a Quantity.

I've been investigating different approaches:

  • Making Sample type parameter bound to numbers.Real. This didn't work because brilliantly, isinstance(1.0, Real) is True, but using T = TypeVar("T", Real) is not good enough to the pass float as T (the types don't match). This seems to be a know bug that will not be fixed, as it seems numbers were created before type hints to solve some other problem. In any case, it will be also very hard to make Quantity be a proper Real, because we can't allow all operations (for example, if we multiply a quantity by the same type of quantity, we get quantity², not quantity, so we can't generally allow it.

  • Building our own RestrictedFloat type, or something like that, to make it compatible with float. This sort of works, but when looking at how much restricted a float needs to be to be a Quantity, there is not much left. It is basically comparison, addition and scaling.

  • Making Quantity convertible to float (implement __float__()). This seems to be the most sensible solution. By doing this the most important operations in math related to floats that we need to do in the most generic places even work without conversion, like math.isnan() and math.isinf(), so we can treat Quantitys as float directly for that and remove the isnan() and isinf() methods, being able to work with float and Quantity in a generic way. For other math, we can do v = float(quantity) and then do whatever math we need with v in a generic way. Of course we generally don't want users to convert to float, this is why we have all the as_xxx() methods, but when used generically, like in the resampler, we know we are working with the same type of quantity always, so it is safe to convert to float. Also making operations that escape the quantity (like v * v), should not make sense in resampling functions, because the new sample always need to be in the same quantity as the input samples, so that should never happen because of how math works.

@llucax llucax added type:enhancement New feature or enhancement visitble to users part:data-pipeline Affects the data pipeline part:actor Affects an actor ot the actors utilities (decorator, etc.) part:core Affects the SDK core components (data structures, etc.) scope:breaking-change Breaking change, users will need to update their code labels Dec 18, 2023
@llucax llucax added this to the v1.0.0-rc4 milestone Dec 18, 2023
@llucax llucax self-assigned this Dec 18, 2023
github-merge-queue bot pushed a commit that referenced this issue Jan 10, 2024
The channel registry is using `Any` as the message type, which is not
type safe, as it completely *disables* type checking for the messages.

This commit makes the channel registry type-aware, so channels are
stored with their message type and the registry checks that the same
channel is not used for different message types.

This also makes the registry just a plain container for channels, the
wrapper methods to create new senders and receivers, and to configure
the `resend_latest` flag are removed. The `ReceiverFetcher` abstraction
is also not needed if we just return the channel directly, as the
channel itself is a `ReceiverFetcher`.

Also the method to close and remove a channel is made public and the
name more explicit, as it is used in normal code paths.

The new registry only provide 2 main methods:

* `get_or_create()`: Get or create a channel for the given key, doing
the type checking to make sure the requested message type matches the
existing channel message type if it already exists.

* `close_and_remove()`: Close and remove the channel for the given key.

This change uncovered 5 issues:

* `MockMigrogrid/Resampler: Fail on unhandled exceptions` (in this PR,
but more generally #826)
* `Fix checks in tests` (in this PR)
* `Fix missing conversion to `QuantityT` (workaroud in this PR, but
should be better solved by #821)
* #807
* #823

Fixes #806.
@llucax llucax modified the milestones: v1.0.0-rc4, v1.0.0-rc5 Feb 1, 2024
github-merge-queue bot pushed a commit that referenced this issue Feb 13, 2024
In Python `float` is a bit weird, because `isinstance(1, float)` is
`False` but still `int` is a subtype of `float` in terms of the type
system, so `int` will be accepted when a `float` is specified in
arguments for example.

This leads to quantities that can have `int` as base value if it was
constructed with a `int` instead of a `float`, which is unintuitive when
the `base_value` type hint is `float` and can potentially cause
problems, in particular when addressing #821.
@llucax llucax modified the milestones: v1.0.0-rc5, v1.0.0-rc6 Feb 19, 2024
@llucax llucax modified the milestones: v1.0.0-rc6, v1.0.0-rc7 Mar 26, 2024
@llucax llucax modified the milestones: v1.0.0-rc7, post-v1.0 Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:actor Affects an actor ot the actors utilities (decorator, etc.) part:core Affects the SDK core components (data structures, etc.) part:data-pipeline Affects the data pipeline scope:breaking-change Breaking change, users will need to update their code type:enhancement New feature or enhancement visitble to users
Projects
Status: To do
Development

Successfully merging a pull request may close this issue.

1 participant