Parse Quantity Strings #824

Marenz · 2023-12-19T17:38:14Z

Fix missing number in Quantity formatting for small values
Add function to allow parsing of Quantity strings

llucax

LGTM otherwise.

tests/timeseries/test_quantities.py

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz · 2024-01-29T18:48:23Z

Updated and ready for review

llucax · 2024-01-30T08:23:55Z

CI failing with:

pylint src benchmarks docs examples noxfile.py tests
************* Module frequenz.sdk.timeseries._quantities
src/frequenz/sdk/timeseries/_quantities.py:204:4: R0912: Too many branches (16/12) (too-many-branches)
************* Module tests.timeseries.test_quantities
tests/timeseries/test_quantities.py:582:4: W0212: Access to a protected member _base_value of a client class (protected-access)

llucax

It looks like the second commit could be split, other than that (and the CI failure) LGTM.

src/frequenz/sdk/timeseries/_quantities.py

tests/timeseries/test_quantities.py

Marenz · 2024-01-30T09:04:19Z

CI failing with:..

haha, I had that fixed, I just didn't add it to the commit XD

llucax · 2024-01-31T16:23:21Z

Please re-request a review from me when this is ready for another round.

llucax

For "Fix quantity formatting bug with -0/0" it might be good to describe what the bug actually was, is not completely clear to me by just reading the diff.

About the rounding issue when formatting, really tricky, I wonder if it wouldn't be simpler to do a string roundtrip in the formatting function itself, like convert to string, convert again to float, and only then check which exponent is more appropriate, then convert again to string. It might be less efficient (maybe?) but it seems easier to implement and likely more robust.

llucax · 2024-01-31T16:31:40Z

tests/timeseries/test_quantities.py

+@pytest.mark.parametrize("quantity_type", [Power, Voltage, Current, Energy, Frequency])
+@pytest.mark.parametrize("exponent", [0, 3, 6, 9])
+@hypothesis.settings(max_examples=1000)
+@hypothesis.seed(42)  # Seed that triggers a lot of problematic edge cases


Do you really want to leave the seed fixed? Isn't it more useful to let it change to potentially catch more random cases?

I know for a fact that there are more cases and solving those requires an unreasonable amount of work (at this point).
@sahas-subramanian-frequenz mentioned an imperfect solution is better than no solution at all and at least here I would agree.

There will be cases were it prints slightly weird, maybe using a sub-optimal unit etc, but the printed result will never be straight out wrong.

Yeah, that sounds completely reasonable and I agree 100%. Can you add a comment summarizing this here? Because for the uninformed casual reader it will be really hard to figure all of this out, they'll probably just try to remove those two lines and be completely puzzled about why everything breaks now. I know future me will attempt that 😆

Sorry to be a pain in the ass about this, but I mean something more like "we are setting these fixed because there are still corner cases that will fail if we allow hypothesis to do random tests, this should be removed when all the corner cases are properly handled".

You did see the larger docstring of the function explaining basically that in other words?

No, sorry, I expected it in the comments instead of the docstring so my brain stopped parsing after I couldn't find the clarification there.

tests/timeseries/test_quantities.py

Marenz · 2024-01-31T17:01:42Z

About the rounding issue when formatting, really tricky, I wonder if it wouldn't be simpler to do a string roundtrip in the formatting function itself, like convert to string, convert again to float, and only then check which exponent is more appropriate, then convert again to string. It might be less efficient (maybe?) but it seems easier to implement and likely more robust.

Yes, I thought about this particular solution as well. It might be less elegant, but it would be pretty reliable and covering pretty much all possibilities.
Maybe in a future PR?

Marenz · 2024-01-31T17:02:19Z

For "Fix quantity formatting bug with -0/0" it might be good to describe what the bug actually was, is not completely clear to me by just reading the diff.

Basically it would only print the unit, no number (the strip removed the 0)

llucax · 2024-01-31T17:07:26Z

Maybe in a future PR?

Sure

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

llucax

One last comment about the comment. Also if you can add this "It would only print the unit, no number" to the "Fix quantity formatting bug with -0/0 " commit while you are it, I think it's good info to have in the commit message.

llucax · 2024-02-01T08:18:19Z

tests/timeseries/test_quantities.py

+@pytest.mark.parametrize("quantity_type", [Power, Voltage, Current, Energy, Frequency])
+@pytest.mark.parametrize("exponent", [0, 3, 6, 9])
+@hypothesis.settings(max_examples=1000)
+@hypothesis.seed(42)  # Seed that triggers a lot of problematic edge cases


Sorry to be a pain in the ass about this, but I mean something more like "we are setting these fixed because there are still corner cases that will fail if we allow hypothesis to do random tests, this should be removed when all the corner cases are properly handled".

It would only print the unit, no number. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Certain numbers would be rendered wrongly due to rounding in the python formatter, e.g. Voltage.from_volts(999.9999850988388) would render "1000 V". This results in a faulty round-trip conversion (`num -> str -> num` no longer matching). Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz · 2024-02-01T09:31:29Z

Extended the commit description with the "no unit" explanation

llucax · 2024-02-01T09:39:26Z

Maybe in a future PR?

Quantity.__format__() can sometimes produce wrong results frequenz-quantities-python#11

Marenz requested a review from a team as a code owner December 19, 2023 17:38

Marenz requested a review from llucax December 19, 2023 17:38

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 Dec 19, 2023

Marenz force-pushed the parse_quantity branch from c53ad85 to d23cd33 Compare December 19, 2023 17:45

llucax assigned Marenz Dec 20, 2023

llucax added this to the v1.0.0-rc4 milestone Dec 20, 2023

llucax reviewed Dec 20, 2023

View reviewed changes

tests/timeseries/test_quantities.py Show resolved Hide resolved

tests/timeseries/test_quantities.py Outdated Show resolved Hide resolved

Marenz force-pushed the parse_quantity branch from d23cd33 to cebfd65 Compare December 20, 2023 08:40

Fix missing number in Quantity formatting for small values

cc10827

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the parse_quantity branch from cebfd65 to 36b0205 Compare January 29, 2024 18:47

github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Jan 29, 2024

llucax reviewed Jan 30, 2024

View reviewed changes

Marenz force-pushed the parse_quantity branch 2 times, most recently from 1bae2f7 to 1d300b4 Compare January 30, 2024 10:27

Marenz requested a review from llucax January 31, 2024 16:23

llucax reviewed Jan 31, 2024

View reviewed changes

Add function to allow parsing of Quantity strings

1123f46

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the parse_quantity branch from 1d300b4 to 964669f Compare January 31, 2024 17:16

Marenz requested a review from llucax January 31, 2024 17:16

llucax reviewed Feb 1, 2024

View reviewed changes

llucax approved these changes Feb 1, 2024

View reviewed changes

Marenz added 2 commits February 1, 2024 10:29

Fix quantity formatting bug with -0/0

493df59

It would only print the unit, no number. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the parse_quantity branch from 964669f to 34961ec Compare February 1, 2024 09:30

Marenz enabled auto-merge February 1, 2024 09:31

Marenz added this pull request to the merge queue Feb 1, 2024

Merged via the queue into frequenz-floss:v1.x.x with commit 971d7cf Feb 1, 2024
14 checks passed

Marenz deleted the parse_quantity branch February 1, 2024 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse Quantity Strings #824

Parse Quantity Strings #824

Marenz commented Dec 19, 2023

llucax left a comment

Marenz commented Jan 29, 2024

llucax commented Jan 30, 2024

llucax left a comment

Marenz commented Jan 30, 2024

llucax commented Jan 31, 2024

llucax left a comment

llucax Jan 31, 2024

Marenz Jan 31, 2024

llucax Jan 31, 2024

llucax Feb 1, 2024

Marenz Feb 1, 2024

llucax Feb 1, 2024

Marenz commented Jan 31, 2024

Marenz commented Jan 31, 2024

llucax commented Jan 31, 2024

llucax left a comment

llucax Feb 1, 2024

Marenz commented Feb 1, 2024

llucax commented Feb 1, 2024

Parse Quantity Strings #824

Parse Quantity Strings #824

Conversation

Marenz commented Dec 19, 2023

llucax left a comment

Choose a reason for hiding this comment

Marenz commented Jan 29, 2024

llucax commented Jan 30, 2024

llucax left a comment

Choose a reason for hiding this comment

Marenz commented Jan 30, 2024

llucax commented Jan 31, 2024

llucax left a comment

Choose a reason for hiding this comment

llucax Jan 31, 2024

Choose a reason for hiding this comment

Marenz Jan 31, 2024

Choose a reason for hiding this comment

llucax Jan 31, 2024

Choose a reason for hiding this comment

llucax Feb 1, 2024

Choose a reason for hiding this comment

Marenz Feb 1, 2024

Choose a reason for hiding this comment

llucax Feb 1, 2024

Choose a reason for hiding this comment

Marenz commented Jan 31, 2024

Marenz commented Jan 31, 2024

llucax commented Jan 31, 2024

llucax left a comment

Choose a reason for hiding this comment

llucax Feb 1, 2024

Choose a reason for hiding this comment

Marenz commented Feb 1, 2024

llucax commented Feb 1, 2024