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.__format__() can sometimes produce wrong results #11

Open
llucax opened this issue Feb 1, 2024 · 5 comments
Open

Quantity.__format__() can sometimes produce wrong results #11

llucax opened this issue Feb 1, 2024 · 5 comments
Labels
priority:low This should be addressed only if there is nothing else on the table type:bug Something isn't working
Milestone

Comments

@llucax
Copy link
Contributor

llucax commented Feb 1, 2024

What happened?

The main issue seems to be that we are doing the calculation about which exponent to use based on the base_value, but then when we convert the base_value to string, the float string conversion does some rounding, that could change how many significant digits we end up with.

For example: if base_value for power is 999.9999850988388, then we consider it is smaller than the next possible exponent (1kW) so we use just the base unit (W), but when doing str(base_value), this number ends up displayed as "1000", so we return "1000 W" when we should have returned "1 kW". This specific case is now resolved, but there are other that prevent, in particular, for this invariant to always be true: Power.from_string(str(power)) == power.

What did you expect instead?

Power.from_string(str(power)) == power always.

Affected version(s)

No response

Affected part(s)

Core components (data structures, etc.) (part:core)

Extra information

The current implementation to handle these weird corner cases is quite complex, and it doesn't seem to be handling all of them. An, less efficient, alternative would be to just do a string round-trip before calculating what's the most appropriate exponent to use, so convert to string, convert again to float, and only then check which exponent is more appropriate, then convert again to string.

@llucax llucax added the type:bug Something isn't working label Feb 1, 2024
@llucax llucax added this to the v1.0.0 milestone Feb 1, 2024
@llucax
Copy link
Contributor Author

llucax commented Feb 1, 2024

Thinking more about this, the test is wrong. We can't possibly always ensure that Power.from_string(str(power)) == power is true if we are rounding, this doesn't work for float either:

>>> f = 0.1001
>>> f"{f:.3}"
'0.1'
>>> f = 0.1001
>>> f"{f:.3}"
'0.1'
>>> f2 = float(f"{f:.3}")
>>> f == f2
False

So maybe @Marenz it would be useful if you can mention any cases you find that are currently failing, and also we might need to fix the tests.

@Marenz
Copy link
Contributor

Marenz commented Feb 1, 2024

the comparison is more whether the produced string is the same, not the base value

@llucax
Copy link
Contributor Author

llucax commented Feb 1, 2024

I didn't get that.

@Marenz
Copy link
Contributor

Marenz commented Feb 1, 2024

What did you expect instead?
Power.from_string(str(power)) == power always.

str(Power.from_string(str(power))) == str(power)

@llucax
Copy link
Contributor Author

llucax commented Feb 1, 2024

Mmmmm, I see, still I can see how that might be flaky if Power.from_string(str(power)) == power doesn't hold. But maybe it is still safe...

@llucax llucax removed this from the v1.0.0 milestone Feb 5, 2024
@llucax llucax added the priority:low This should be addressed only if there is nothing else on the table label Apr 9, 2024
@llucax llucax transferred this issue from frequenz-floss/frequenz-sdk-python Jun 26, 2024
@llucax llucax added this to the Untriaged milestone Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:low This should be addressed only if there is nothing else on the table type:bug Something isn't working
Projects
Status: To do
Development

No branches or pull requests

2 participants