Skip to content
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.

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

Element.modular math is wrong (easy fix provided) #331

Open
andreacfromtheapp opened this issue Jan 7, 2022 · 0 comments
Open

Element.modular math is wrong (easy fix provided) #331

andreacfromtheapp opened this issue Jan 7, 2022 · 0 comments
Labels
has-ellie This is a bug and has an ellie which demonstrates it.

Comments

@andreacfromtheapp
Copy link

andreacfromtheapp commented Jan 7, 2022

Hi and thank you for elm-ui 😄

Element.modular formula is off because of the use of -1.

Current behavior

Each multiplier returns what the previous should:

  • E.g: Element.modular 16 1.25 1 yields 16 instead of 20
  • E.g: Element.modular 16 1.25 2 yields 20 instead of 25
  • E.g: Element.modular 16 1.25 3 yields 25 instead of 31.25
  • and so on

Expected behavior

With the above in mind, and with a base, a ratio, and a multiplier:

  • Only 0 for the multiplier should yield the base number unchanged. (16 in this example).
  • Element.modular 16 1.25 1 should yield 20 and not 16. (try 16*1.25*1 in a calc)

Versions

  • Elm Version: 0.19.1
  • Elm UI Version 1.1.8

Solution

The issue is caused by using the -1 in the function definition. The fix is super easy and saves an else if that's not needed because the negative multiplier suffice:

modular : Float -> Float -> Int -> Float
modular normal ratio rescale =
    if rescale == 0 then
        normal

    else
        normal * ratio ^ toFloat rescale

I put a demo of this on Ellie: https://ellie-app.com/gm2bfCbKdtZa1

Also refer to these to corroborate my findings:

Docs Need Updating too

Contextually, the documentation examples should be fixed/updated as well, to match the corrected formula. Also, to add partial application to the (scaled) examples. Ref. #36

Cheers :)

ps: if needed I would gladly do the PR(s) 💯

@github-actions github-actions bot added the has-ellie This is a bug and has an ellie which demonstrates it. label Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-ellie This is a bug and has an ellie which demonstrates it.
Projects
None yet
Development

No branches or pull requests

1 participant