microsoft / pxt-microbit

A Blocks / JavaScript code editor for the micro:bit built on Microsoft MakeCode
https://makecode.microbit.org
Other
727 stars 640 forks source link

Error in calculating power function #2192

Closed shaoziyang closed 3 months ago

shaoziyang commented 5 years ago

Describe the bug I calculate a expression in makecode, then send the value to serial port. In simulator it is ok, but in real serial port, it will get wrong number.

let P = 100240
basic.forever(function () {
    serial.writeValue("T", (P / 101325) ** (1 / 5.255))
    basic.pause(1000)
})

In simulator, it will show:

T:0.997953407897062

But in real serial port it will show:

T: 1

shaoziyang commented 5 years ago

If Multiply by 100

let P = 100240
basic.forever(function () {
    serial.writeValue("T", (P / 101325) ** (1 / 5.255) * 100)
    basic.pause(1000)
})

Simulator will show:

T:99.7953407897062

And real serial port will show:

T: 100

shaoziyang commented 5 years ago

I have make a simple test, get same result:

https://makecode.microbit.org/_ixr8jjaYTeM6

whaleygeek commented 5 years ago

Implementation of floats differ slightly between the Typescript and the micro:bit runtime library, are these just 'small rounding differences'?

Might be related to: https://github.com/microsoft/pxt-microbit/issues/1781

whaleygeek commented 5 years ago

Hmmm, @abchatra looks suspiciously more like a type conversion error, given that the number on the real micro:bit comes out without any decimal places?

mmoskal commented 5 years ago

The power function on microbit is rounding the exponent to an integer. Regular pow() function brings in 4k of code, for which I think we don't have space in flash. This is controlled by PXT_POWI configuration macro.

We should definitely align sim with hw. Maybe we should just error on the ** operator when PXT_POWI is defined.

Note that this is the case only on micro:bit (and in light mode on maker, which currently only applies to bluepill). Other targets, including Arcade and CPX, use real pow function.

mmoskal commented 5 years ago

Also, there should be no differences in the actual values of numbers between simulator and hardware; hardware may round them differently but only when displaying, the intermediate results should be exactly the same.

BTW, it seems the fix for displaying numbers never made it into microbit release branch.

whaleygeek commented 5 years ago

FYI, the fix for displaying correctly rounded numbers is quite important, I have had lots of people moan at me about how the micro:bit got harder to use when floating point was added, and the root cause seems to be display rounding.

On Thu, 25 Jul 2019, 17:51 Michał Moskal, notifications@github.com wrote:

Also, there should be no differences in the actual values of numbers between simulator and hardware; hardware may round them differently but only when displaying, the intermediate results should be exactly the same.

BTW, it seems the fix for displaying numbers https://github.com/microsoft/pxt-common-packages/pull/862 never made it into microbit release branch.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/microsoft/pxt-microbit/issues/2192?email_source=notifications&email_token=AA7FYANQZXKXZBAGXROGNTDQBHKZTA5CNFSM4HX4AMP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD22CBLQ#issuecomment-515121326, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7FYAOYY47PLPJNUDZCGDDQBHKZTANCNFSM4HX4AMPQ .

pelikhan commented 5 years ago

@abchatra fyi

CPH-Scot commented 4 years ago

Hi All,

I have run into the same problem as shaoziyang above and get the "1" returned when I try the exponent function in Microbit. Its not clear to me though from this thread if there is a workaround that could be used or a fix coming or indeed if it is simply impossible to do this in Microbit- can any help clarify? My apologies if this is somehow clear as I am quite new to this (my very first post/question :-)) !

Thanks, Ian

shaoziyang commented 4 years ago

Hi All,

I have run into the same problem as shaoziyang above and get the "1" returned when I try the exponent function in Microbit. Its not clear to me though from this thread if there is a workaround that could be used or a fix coming or indeed if it is simply impossible to do this in Microbit- can any help clarify? My apologies if this is somehow clear as I am quite new to this (my very first post/question :-)) !

Thanks, Ian

I use the series method for approximate calculation.

https://github.com/shaoziyang/xinabox-pxt-sw01/blob/master/sw01.ts

CPH-Scot commented 4 years ago

Hi All, I have run into the same problem as shaoziyang above and get the "1" returned when I try the exponent function in Microbit. Its not clear to me though from this thread if there is a workaround that could be used or a fix coming or indeed if it is simply impossible to do this in Microbit- can any help clarify? My apologies if this is somehow clear as I am quite new to this (my very first post/question :-)) ! Thanks, Ian

I use the series method for approximate calculation.

https://github.com/shaoziyang/xinabox-pxt-sw01/blob/master/sw01.ts

Works perfectly Shaoziyang! - might modify the code for my application to measure relative altitude depending on local pressure (reference). Many thanks!

abchatra commented 4 years ago

thanks @shaoziyang for the workaround

abchatra commented 4 years ago

This should be fixed in /beta? If not let me know.

mmoskal commented 4 years ago

I believe this is still broken. We need to make a call if the simulator should be updated to match hardware or if we should just drop the ** operator in micro:bit - I would lean towards the second option.

microbit-matt-hillsdon commented 4 years ago

A user just raised this via micro:bit support ticket 28852.

gustavvvo2 commented 4 years ago

I found the same issue trying to implement a function to translate analog input value from a CNY70 to centimetres. d = 849.4548 * v * -0.7475094 Due to this bug, I tried to do this approach d = 849.4548 v (-3/4) So I implemented this function using Makecode block editor: d= 849.4548 / ( sqrt(sqrt(v 3)) ) It works, as sqrt() uses floats in hardware calculations. I hope this way to approach could be useful for someone... at least, until someone fix this bug.

microbit-mark commented 3 years ago

Also raised in micro:bit support ticket 43572 when trying to output a musical scale

https://makecode.microbit.org/_5mUFiocM6hFz

let pitch = 440
for (let index = 0; index < 12; index++) {
    music.playTone(pitch, music.beat(BeatFraction.Whole))
    pitch = Math.round(pitch * 2 ** (1 / 12))
    serial.writeValue("p", pitch)
}

Works in sim but constantly outputs 440 on the hardware

Can we revisit this given the comments and workarounds in this issue @abchatra

kevinjwalters commented 1 year ago

I just discovered this the hard way. It would be quicker to work out if MakeCode threw ones of those numbered halting exceptions and if the simulator had same behaviour as micro:bit as debugger is misleading.

It was also reported in 2021 in the forums in MakeCode Forum: Math.pow floating point issues.

CaelCoruscare commented 7 months ago

Here in 2024, this is still an issue. It also looks to be a repeat of the 2018 issue https://github.com/Microsoft/pxt-microbit/issues/1435 , which was marked as "completed" and closed by @abchatra in 2020.

Also, to weigh in on the "should we fix it, or just drop exponents from MicroBit" discussion, I'm here because someone I know is hundreds of hours into creating school material for highschoolers using MicroBit, intended to be widely used, and has now discovered that the Exponents are broken and Logarithms are not implemented except in third party libraries, while designing the lesson plan on Logarithms and Exponents.

CaelCoruscare commented 7 months ago

I did some more testing, and I found that the issue happens only if:

  1. You use a fraction in the exponent
  2. The fraction is the result of a division expression

image

The second and third actions in the image above work as expected. The first action does not.

That is to say, according to the Microbit: 10 2 = 100 10 1 = 10 10 1.5 = 31 10 (3/2) = 10

A fraction inside the exponent, breaks the exponent. Probably the fraction is converted to an int before it is passed to the exponent? Even though you can pass a float explicitly and it will work as expected?

Here is the Javascript Code, in case that helps: image

Also, in case it helps, MicroBit version is v2.21

CaelCoruscare commented 7 months ago

I have also confirmed that storing a float in a variable, and then putting the variable into an exponent, breaks the exponent. As described in this forum post from 2021 with no resolution: https://forum.makecode.com/t/math-pow-floating-point-issues/6305

jaustin commented 7 months ago

@abchatra having read through the issue here I think we need to come to a decision for this release about the behaviour.

Anyone revisiting this issue, https://github.com/microsoft/pxt-microbit/issues/2192#issuecomment-2046115563 has a good up-to-date summary.

One one hand, the 4K of flash we were saving is probably much less relevant in a majority V2 world, but having differences in fundamental behaviour between V1 and V2 is more confusing.

I think @mmoskal's older suggestion of dropping the ** operation might have been viable in the past but I suspect we're too entrenched now to remove it. Could we throw an error at the point an exponent is rounded (still a behaviour change, but at least explicit)

Broadly I think we'll need to discuss this. Would like to here perspectives from folks - and from the Foundation team specifically @microbit-giles and @microbit-carlos for the edu resource and flash consumption points respectively.

microbit-carlos commented 7 months ago

I agree that dropping ** is not an option anymore.

4K for pow() is a lot, is that linked on every build, or only on builds using pow()?

If it's 4K used in all programmes (including those not using the power function), my suggestion would be to update the simulator to behave like the hardware and then create a float version in an extension. This would make V1, V2 and simulator all work the same way, and still be able to use floats in the programs that can spare the extra resources.

If the 4K are only added when using the function, then it's a bit less clear. V1 and V2 compatibility is probably the top priority and the extra flash consumption is hard penalty for V1. Do we know how much free flash do with have in total in V1?

(Assuming the simulator and hardware will behave the same) This is also the kind of update that could either be consider a bug fix or a breaking change. If existing user programmes rely on the existing behaviour it will be a breaking change for them, even if by some users it could be considered a bug fix. This makes me lean a bit more towards having an extra block (in the toolbox or an extension) for floats, instead of updating the current version that does the int conversion, but I'm still on the fence on this one.

jaustin commented 7 months ago

On discussion with @abchatra today we think a good solution could be

Then the question is what to do with the inconsistency around literals? Is this the Typescript compiler optimising for some literals but not going as far as doing the division?

martinwork commented 7 months ago

@jaustin Yes. I experimented for a support ticket.

See https://github.com/microsoft/pxt-microbit/issues/4436#issuecomment-1774152111

There's a purely typescript solution using exp(log()), which is slow compared to C++ pow. I was also thinking about adding blocks for log, exp, trig etc.

Will any extension that exposes C++ pow add the 4K, even if the project doesn't use it? If so maybe we need a single function extension!

martinwork commented 7 months ago

Could the exp(log()) calculation be used for non-integer exponents in the PXT_POWI variant?

pelikhan commented 7 months ago

C++ is not tree shakes so yes you want that 1 function extension.

Won't be the first... https://www.npmjs.com/package/is-true

kevinjwalters commented 7 months ago

@CaelCoruscare I borrowed the code from the ZX Spectrum to do exp in MakeCode. That might provide a workaround for the interim. Log could be done in the same style.

Described in https://www.instructables.com/Rainbows-in-MakeCode-on-a-4tronix-CubeBit-RGB-LED-/ which has link to https://makecode.microbit.org/_RUHaj1P1rAVW where you'll find expapprox

martinwork commented 7 months ago

Thanks @pelikhan - saved me testing it!

martinwork commented 7 months ago

In my existing experiment with calling C++ pow, the ts function calls Math.pow() for the simulator. That won't work if the simulator's Math.pow() is "fixed" to match the current PXT_POWI implementation. I don't know how to add a new implementation for the simulator. For that code, matching simulator pow() to micro:bit, is a breaking change.

I noticed this: image

martinwork commented 7 months ago

Could something like the javascript function below provide a built in "x to power y" block for fractional exponents?

It seems the speed advantage of my experiment with C++ pow was due to using the float overload, and only on V2.

Using toDouble/fromDouble like https://github.com/microsoft/pxt-common-packages/blob/38e15f17cb02d588c30983c0167451457ce62b29/libs/base/core.cpp#L1342 the result seems to be slower than the javascript powfn below, though I don't understand why!

Using toFloat/fromFloat, it's faster than powfn on V2 but slower on V1.

My test of powfn vs C++: https://makecode.microbit.org/_eobRDfYTK5C7 Timings for V2: powfn = 1621; C++ = 410 and V1: powfn = 14430; C++ = 27761

Of course, x ** y is fastest!

function powfn(x: number, y: number): number {
    if (x > 0) {
        return Math.exp(y * Math.log(x))
    }
    if (x < 0 && Math.round(y) != y) {
        return Math.log(x)
    }
    return Math.pow(x, y);
}
pelikhan commented 7 months ago

Does performance matter? Seems like correctness is what we care about here.

martinwork commented 7 months ago

If performance doesn't matter, could we simply fix the PXT_POWI implementation by using Math.exp(y * Math.log(x))? @jaustin @microbit-carlos

https://github.com/martinwork/pxt-math-power/blob/master/math-power.cpp#L15

riknoll commented 3 months ago

I agree with @martinwork, we should just fix the PXT_POWI implementation. Correctness is much more important than perf.

@martinwork would you like to open a PR in pxt-common-packages? I'll review/merge it. Otherwise, I can do the PR myself based off of your code.

microbit-carlos commented 3 months ago

I'm still a bit concern about having a 4K flash penalty in all builds. Can this be fixed in a way that only programmes that use this block will have the extra flash consumption?

riknoll commented 3 months ago

@microbit-carlos the solution from @martinwork doesn't have the 4k flash penalty. It only uses the existing exp and log functions which are already included in all builds, at the cost of being less performant than the native pow

martinwork commented 3 months ago

@microbit-carlos @riknoll My suggestion was to use the Math.exp(y * Math.log(x)) code to fix the powi implementation, not to add in the C++ pow by removing PXT_POWI, like https://github.com/microsoft/pxt-microbit/pull/5849 seems to.

@riknoll Please go ahead with whatever is agreed!

microbit-carlos commented 3 months ago

Ah, awesome, thanks for the clarification!

riknoll commented 3 months ago

yeah, I opened #5849 which removed PXT_POWI for v2 before seeing this thread. I'll open a new PR with the discussed change