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

scaleTo methods for Vector2d and Vector3d #137

Merged
merged 14 commits into from
Oct 12, 2020

Conversation

g-belmonte
Copy link
Contributor

Pull request related to #134

Pull request checklist:

  • Is the pull request from a dedicated branch in your fork instead of
    master?
  • Have you added yourself to the AUTHORS file?
  • Is "Allow edits from maintainers" enabled?
  • Is the code formatted using the same version of elm-format as the rest of
    elm-geometry?
  • Is code (mostly) wrapped to 80 columns?
  • BONUS POINTS: Have you added tests for new functionality?
  • EXTRA BONUS POINTS: Have you added documentation for new functionality?

@ianmackenzie let me know what you think! :)

@ianmackenzie
Copy link
Owner

Looking good, thanks for adding docs/tests! A few requests:

  • Can you try implementing scaleTo similar to the old normalize? For performance reasons I'd like to avoid the intermediate Maybe (extra memory allocation) if possible. I think this will involve adding something like scaleFactor = givenLength / scaledLength to the existing normalize logic and then use x = scaleFactor * scaledX instead of x = scaledX / scaledLength.
  • I think those tests would fail if the fuzzer produced a zero vector (which you can probably test by increasing the number of fuzz tests that get run, e.g. elm-test --fuzz 5000). I think there should probably be some logic in both the scaleTo and normalize tests that first checks if the fuzzed vector is equal to Vector2d.zero/Vector3d.zero and if so checks that the scaled vector is also equal to the zero vector; otherwise use the existing logic.
  • Very minor point but I think the tests would be slightly cleaner if you used Test.fuzz2 instead of Test.fuzz with Fuzz.tuple.

@g-belmonte
Copy link
Contributor Author

g-belmonte commented Oct 11, 2020

I did some changes to the scaleTo, then I ran elm-test --fuzz 5000. This is what I got:

image

Is it something to worry about? 😅

(If you want to run it: elm-test --fuzz 5000 --seed 5581032459802)

@ianmackenzie
Copy link
Owner

Yeah, don't worry about that test failure, I think I just set it up in a way that you occasionally have really numerically bad situations (like an axis that's almost parallel to the plane) where it's impossible to get super high accuracy no matter what you do. I do have to fix that test to avoid those cases (while being careful not to filter out actual problems), but it's certainly not related to anything you're doing!

@ianmackenzie
Copy link
Owner

By the way, any chance you could tweak the Vector3d example to be a bit more like the Vector2d one? (Nicer numbers, output is formatted as an expression you could actually write instead of what the REPL would show, etc.) You can 'cheat' a bit here and basically use the same 2D example by just (for example) setting the Y component to zero in the 3D case.

@ianmackenzie
Copy link
Owner

For the tests, it's certainly good to have some explicit tests for the zero vector case, but I think there still needs to be something in the general case to handle the situation where the fuzzer produces a zero vector (I may not have explained this well). For example, taking normalize as an example because it's a bit simpler,

\vector ->
    Vector3d.normalize vector
        |> Vector3d.length
        |> Expect.quantity (Quantity.float 1)

will fail if vector happens to be Vector3d.zero since then the length will be zero, not one! I think you'll need something like

\vector ->
    if vector == Vector3d.zero then
        Vector3d.normalize vector
            |> Expect.equal Vector3d.zero
    
    else
        Vector3d.normalize vector
            |> Vector3d.length
            |> Expect.quantity (Quantity.float 1)

@ianmackenzie
Copy link
Owner

Oh, and I appreciate the attempt to simplify/clean up the old implementation of normalize, but there's actually a reason for the weird logic with largestComponent 🙂. Mathematically your version is the same (and simpler), but numerically calculations like x * x can underflow or overflow with very small or very large Float values (see #99 for a detailed discussion). Dividing everything by the largest component (and then effectively multiplying by that again afterwards, after doing the square/square root) avoids both issues.

@g-belmonte
Copy link
Contributor Author

I'm ashamed by the looks of that bad 3d vector example 😂 For the Vector2d I used Pythagorean triplets, so that I could have nice looking numbers... but I had no idea of an example like that on a 3d vector 😅 I'll put one of the components as zero for now

Now I see the problems with the tests and the implementation I did 😄 Let me fix that :)

@ianmackenzie
Copy link
Owner

I think this is looking great now! Was there anything else you were thinking of adding or are you OK to have this merged?

@g-belmonte
Copy link
Contributor Author

I think this is it, at least for this one 😁

@ianmackenzie ianmackenzie merged commit 461cf7b into ianmackenzie:master Oct 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants