microbit-foundation / micropython-microbit-v2

Temporary home for MicroPython for micro:bit v2 as we stablise it before pushing upstream
MIT License
41 stars 22 forks source link

`microbit.scale()` returns a float from integer inputs #121

Closed microbit-carlos closed 1 year ago

microbit-carlos commented 1 year ago

For example, with code like this we get an error as scale() is returning a float:

from microbit import *

my_effect = audio.SoundEffect()

while True:
    my_effect.freq_start = scale(accelerometer.get_x(), from_=(-2000, 2000), to=(0, 9999))
    my_effect.freq_end = scale(accelerometer.get_y(), from_=(-2000, 2000), to=(0, 9999))
    audio.play(my_effect)
dpgeorge commented 1 year ago

I did consider the integer/float return case. But I don't see an easy way to define scale() if sometimes it returns an integer. Do you have an idea how it should behave?

microbit-carlos commented 1 year ago

The main use-case we have for the scale() function is to map different ranges between input (sensors) and output (display, speaker) sources, and I believe they all use integers, so having integer as the default output type would be useful, otherwise the majority of uses will be wrapped by int(), eg. int(scale(...))

But I don't see an easy way to define scale() if sometimes it returns an integer.

Do you mean how to define typings? Or something else?

Would it be possible to return an integer if the input value and the "to" arguments are integers? And a float if any of those is a float?

I guess in this case we'd still have two options for the internal calculations: To do them as integers or to calculate as floats and covert (or not) the final value. The second option (calc floats, convert to int if necessary) is more accurate but less performant, but might be required or in some cases integer division might not be good enough.

Other than performance, would there be any disadvantages of doing that?

We'd need to document the int vs float output, so if somebody wants to do something like scale(acc.get_x(), from_=(-2000, 2000), to=(0, 1)) they could still get a float output by using the to argument as to=(0.0, 1.0).

I guess an alternative could be to have an additional parameter? Something like scale(..., output=int), so that it converts the output with int() (it could just be a callable, so int, float, or any user define function could work), which could be changed to None or float?

dpgeorge commented 1 year ago

Would it be possible to return an integer if the input value and the "to" arguments are integers? And a float if any of those is a float?

I did initially think this, but then consider (from your comment https://github.com/microbit-foundation/micropython-microbit-v2/issues/102#issuecomment-1216746204):

A good example is when converting between celsius and fahrenheit, as you normally would do this as microbit.scale(value, from=(0, 100), to=(32, 212))

If you pass in an integer value then the output will be an integer and will be (most likely) wrong. Wrong in the sense that it'll be truncated and users will get confused and think the function is broken because it won't match what they expect the answer to be.

they could still get a float output by using the to argument as to=(0.0, 1.0).

This could be a solution, to make it very explicit that the values/types passed to to set the type of the output.

I guess an alternative could be to have an additional parameter?

I think that adds extra complexity which is not needed (although the output function could be a clamping function...).

microbit-carlos commented 1 year ago

they could still get a float output by using the to argument as to=(0.0, 1.0).

This could be a solution, to make it very explicit that the values/types passed to to set the type of the output.

I guess an alternative could be to have an additional parameter?

I think that adds extra complexity which is not needed (although the output function could be a clamping function...).

So having the output being an integer or a float based on the type of the to parameter sounds good to me. So we can have:

microbit.scale(value, from=(0, 100), to=(32.0, 212.0))

Or

scale(accelerometer.get_x(), from_=(-2000, 2000), to=(0, 9999))

I'm thinking if any of the tuple values in to are floats (in case the user does to=(0, 10.0) then the output should be a float. Or in order words, both values have to be integers to convert the output from float to integer.

Does that sound good? Would there be any other simpler ways to approach it, implementation-wise?

dpgeorge commented 1 year ago

I'm thinking if any of the tuple values in to are floats (in case the user does to=(0, 10.0) then the output should be a float. Or in order words, both values have to be integers to convert the output from float to integer.

I have implemented this in 7f9c32e8efd11c9b16e42091c40076b409063ccc (it was quite simple to do).

dpgeorge commented 1 year ago

If we agree on this implementation, micro:bit v1 needs updating as well.

microbit-carlos commented 1 year ago

Thanks Damien! And yes, we will need to update v1 as well with this change 👍

dpgeorge commented 1 year ago

micro:bit v1 is updated.

I think we can close this issue now.