Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Feb 13, 2024

Dividing by float is only applying a factor so it should be safe for any Quantity.

Dividing by Self produces a unit-less ration (float), so it should be safe too.

To be able to use nicer names in overloads, we use positional-only arguments for __truediv__(), which in practice shouldn't be much of a breaking change as these methods should be used mostly via operators.

We also use match syntax in the changed methods.

@llucax llucax requested a review from a team as a code owner February 13, 2024 12:47
@llucax llucax requested a review from Marenz February 13, 2024 12:47
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels Feb 13, 2024
@llucax
Copy link
Contributor Author

llucax commented Feb 13, 2024

@llucax llucax self-assigned this Feb 13, 2024
@llucax llucax marked this pull request as draft February 13, 2024 12:50
Copy link
Contributor

@Marenz Marenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@llucax
Copy link
Contributor Author

llucax commented Feb 14, 2024

See comments

Isn't this a paradox?

@llucax
Copy link
Contributor Author

llucax commented Feb 14, 2024

Thanks for the review, I will update once #875 is merged.

@llucax llucax force-pushed the quantity-div-float branch from 4ba665c to 656f88b Compare February 15, 2024 11:10
@llucax llucax marked this pull request as ready for review February 15, 2024 11:11
@llucax
Copy link
Contributor Author

llucax commented Feb 15, 2024

Rebased and marked the PR ready for review. You comments should be already addressed.

@llucax llucax requested a review from Marenz February 15, 2024 11:11
@llucax
Copy link
Contributor Author

llucax commented Feb 15, 2024

Enabling auto-merge as this had a previous review already.

@llucax llucax enabled auto-merge February 15, 2024 11:12
@llucax llucax force-pushed the quantity-div-float branch from 656f88b to 58ae589 Compare February 16, 2024 13:17
@llucax llucax changed the title Allow all quantities division by float | Percentage | Self Allow all quantities division by float | Self Feb 16, 2024
@llucax
Copy link
Contributor Author

llucax commented Feb 16, 2024

Updated to remove division by Percentage.

Copy link
Contributor

@shsms shsms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few occurances of "percentage" in docstrings that need to be removed, but looks good otherwise.

@llucax llucax force-pushed the quantity-div-float branch from 58ae589 to 375d71d Compare February 19, 2024 09:41
Dividing by `float` is only applying a factor so it should be safe for
any `Quantity`.

Dividing by `Self` produces a unit-less ration (`float`), so it should
be safe too.

To be able to use nicer names in overloads, we use positional-only
arguments for `__truediv__()`, which in practice shouldn't be much of a
breaking change as these methods should be used mostly via operators.

We also use `match` syntax in the changed methods.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax force-pushed the quantity-div-float branch from 375d71d to 15ac8c8 Compare February 19, 2024 09:42
@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

Updated to hopefully remove all the remaining mentions to percentage and rebased.

@llucax llucax requested a review from shsms February 19, 2024 09:42
@llucax
Copy link
Contributor Author

llucax commented Feb 19, 2024

ping @shsms

@llucax llucax added this pull request to the merge queue Feb 19, 2024
Merged via the queue into frequenz-floss:v1.x.x with commit d34512e Feb 19, 2024
@llucax llucax deleted the quantity-div-float branch February 19, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

Development

Successfully merging this pull request may close these issues.

3 participants