Open kepack opened 2 years ago
There should be two distinct units "Hz" and "Angular freq." and they never mix or auto-convert.
On Mon, Jan 31, 2022 at 2:25 PM kepack @.***> wrote:
Describe the bug
The output and input of quantities that have in their unit "2 pi" is broken or rather inconsistent and hence confusing to use. In this example I am creating a variable and set the unit as pure "Hz":
freq_offset = Quantity( value=(-50.5 1e6 2 np.pi), min_val=(-60 1e6 2 np.pi), max_val=(-40 1e6 2 * np.pi), unit='Hz' )
Calling the inbuild functions of Quantity I get the following output:
freq_offset.get_value().numpy() -317300858.0125691
and
freq_offset.get_limits() (-376991118.43077517, -251327412.28718346)
Which both make sense.
Switching to a quantity with unit "Hz 2 pi":
freq_offset2 = Quantity( value=(-50.5 1e6), min_val=(-60 1e6 ), max_val=(-40 * 1e6), unit='Hz 2 pi' )
we now get the following inconsistent output
freq_offset2.get_value().numpy() -158650429.00628456
and
freq_offset2.get_limits() (-60000000.0, -40000000.0)
One can clearly see a mixture of output here. The get_value method returns the value in "Hz" while the get_limits method returns the values in "Hz 2 pi".
On top of that, trying to use the set_value method of Quantity can even crash the programm. As get_value returns the value in "Hz" I initially assumed that the set_value method also expects input in "Hz", but trying to execute the following lines of code:
close_to_max_val_in_hz = -39 1e6 2 * np.pi freq_offset.set_value(close_to_max_val_in_hz)
throws an exception:
Exception: Value -245044226.98000386Hz out of bounds for quantity with min_val: -376.991 MHz and max_val: -251.327 MHz
Expected behavior
It's obvious that this mixture of units for output and input needs to be fixed. It seems to me that the odd one here is the get_value function which is the only function that works in the "Hz" and not the "Hz 2 pi" space.
— Reply to this email directly, view it on GitHub https://github.com/q-optimize/c3/issues/176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH3Q4MQ2ML4WNHX2YR6VILUYZ5VBANCNFSM5NGDOPMQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
@fedroy provided the following example which indicates that the functionality works as intended if there is no space between the "2" and the "pi".
I would nevertheless suggest making the "2 pi"/"2pi" unit feature more relyable so that surprising behaviour as shown above isn't triggered by accident.
Working on this now, with more band-aid coming. To the second point, -39MHz 2 pi
is indeed out of the bounds you specified [-60, -40] MHz 2pi
so the error you're getting at
close_to_max_val_in_hz = -39 * 1e6 * 2 * np.pi
freq_offset.set_value(close_to_max_val_in_hz)
is completely correct.
I agree that we could go through and remove the conversion to explicitly put the 2pi
everywhere.
Note that this is not solved by #187 . As mentioned, this is a kill-it-with-fire situation where we make an executive decision that all frequencies are technical frequencies and include the correct pi or 2 pi where needed. The upside is that testing should be pretty robust for this and point out any places we might miss.
Describe the bug
The output and input of quantities that have in their unit "2 pi" is broken or rather inconsistent and hence confusing to use. In this example I am creating a variable and set the unit as pure "Hz":
Calling the inbuild functions of Quantity I get the following output:
and
Which both make sense.
Switching to a quantity with unit "Hz 2 pi":
we now get the following inconsistent output
and
One can clearly see a mixture of output here. The
get_value
method returns the value in "Hz" while theget_limits
method returns the values in "Hz 2 pi".On top of that, trying to use the
set_value
method of Quantity can even crash the programm. Asget_value
returns the value in "Hz" I initially assumed that theset_value
method also expects input in "Hz", but trying to execute the following lines of code:throws an exception:
Expected behavior
It's obvious that this mixture of units for output and input needs to be fixed. It seems to me that the odd one here is the
get_value
function which is the only function that works in the "Hz" and not the "Hz 2 pi" space.