Skip to content

Make sure base_value is always float #874

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 13, 2024
Merged

Make sure base_value is always float #874

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4d7623
Add a new `_new()` convenience method to `Quantity`
llucax Feb 9, 2024
d7418b9
Make sure `Quantity.base_value` is always a float
llucax Feb 8, 2024
c5364b4
Add clarification about `from_string` tests limitations
llucax Feb 9, 2024
15c0d14
Update release notes
llucax Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions tests/timeseries/test_quantities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ class Fz2(

_CtorType = Callable[[float], Quantity]

# Thi is the current number of subclasses. This probably will get outdated, but it will
# This is the current number of subclasses. This probably will get outdated, but it will
# provide at least some safety against something going really wrong and end up testing
# an empty list. With this we should at least make sure we are not testint less classes
# an empty list. With this we should at least make sure we are not testing less classes
# than before. We don't get the actual number using len(_QUANTITY_SUBCLASSES) because it
# would defeat the purpose of the test.
_SANITFY_NUM_CLASSES = 7
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit late to the review, but it might be better to call this _SANITIFY_NUM_CLASSES.. I just don't see a reason to skip this extra I...

Copy link
Contributor

Choose a reason for hiding this comment

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

just try to pronounce it with and without :P

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree completely. When making typos, please make sure you don't skimp on the vowels. :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea how I ended up typing this. But luckily we have smart completion, so typos are properly propagated and the code never fails!

Expand All @@ -65,7 +65,7 @@ class Fz2(
)
]

# A very basic sainty check that we are messing up the introspection
# A very basic sanity check that are messing up the introspection
Copy link
Contributor

@Marenz Marenz Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

# A very basic sanity check that are messing up the introspection

Someone def. messed up this comment :)

assert len(_QUANTITY_SUBCLASSES) >= _SANITFY_NUM_CLASSES

_QUANTITY_BASE_UNIT_STRINGS = [
Expand All @@ -85,7 +85,7 @@ class Fz2(
and m.__name__ != ("from_string"),
)
]
# A very basic sainty check that we are messing up the introspection. There are actually
# A very basic sanity check that are messing up the introspection. There are actually
# many more constructors than classes, but this still works as a very basic check.
assert len(_QUANTITY_CTORS) >= _SANITFY_NUM_CLASSES

Expand Down Expand Up @@ -609,6 +609,9 @@ def test_invalid_multiplications() -> None:
quantity *= 200.0 # type: ignore


# We can't use _QUANTITY_TYPES here, because it will break the tests, as hypothesis
# will generate more values, some of which are unsupported by the quantities. See the
# test comment for more details.
@pytest.mark.parametrize("quantity_type", [Power, Voltage, Current, Energy, Frequency])
@pytest.mark.parametrize("exponent", [0, 3, 6, 9])
@hypothesis.settings(
Expand All @@ -633,8 +636,14 @@ def test_to_and_from_string(
generation and regenerate it with the more appropriate unit and precision.
"""
quantity = quantity_type.__new__(quantity_type)
# pylint: disable=protected-access
quantity._base_value = value * 10**exponent
quantity._base_value = value * 10**exponent # pylint: disable=protected-access
# The above should be replaced with:
# quantity = quantity_type._new( # pylint: disable=protected-access
# value, exponent=exponent
# )
# But we can't do that now, because, you guessed it, it will also break the tests
# (_new() will use 10.0**exponent instead of 10**exponent, which seems to have some
# effect on the tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

By "on the tests", I guess you mean just this test, because hypothesis also generates some int inputs when we give it a float range?

This function seems to just compare the value from quantity_type.from_string with the value from quantity._base_value. If they don't match up, maybe the from_string method just needs to convert to float as well?

And then they can be compared with quantity.is_close rather than a string comparison?

quantity_str = f"{quantity:.{exponent}}"
from_string = quantity_type.from_string(quantity_str)
try:
Expand Down