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

Fix: is_simple_quad for triangles #35

Merged
merged 1 commit into from
Oct 7, 2023

Conversation

mcnick
Copy link

@mcnick mcnick commented Sep 19, 2023

Problem

I discovered that the area_between_two_curves function is not symmetrical and could even find simple examples where the calculation is obviously wrong, e.g.

p = np.array([[0, 1, 2], [0, 1, 2]]).T
q = np.array([[0, 1, 2], [0, 0, 2]]).T
area_between_two_curves(p, q)  # Output: 0.0
area_between_two_curves(q, p)  # Output: 1.0

It turned out for triangles, i.e. having one vertex twice, the is_simple_quad function is False in case they are in clockwise order.

Changes

  • fixed check for negativity of cross products
  • added tests for triangles

What to look for

  • The logic is still correct and makes sense
    • One could argue is_simple_quad does not need to work for triangles. But the change has no effect on quadrilaterals and for what it's used in this package it makes sense to correctly consider triangles.

@cjekel
Copy link
Owner

cjekel commented Oct 6, 2023

@mcnick Thanks for this! Sorry it's been long. I was unplugged for a couple weeks. My initial thought is something worried about preserving backwards compatibility with the old behavior. However, given that you point how out the old behavior was obviously wrong in some cases then this is a necessary bug fix!

@cjekel
Copy link
Owner

cjekel commented Oct 7, 2023

@mcnick LGTM!

I'm adding your simple triangle example as a basic unit test as well:

p = np.array([[0, 1, 2], [0, 1, 2]]).T
q = np.array([[0, 1, 2], [0, 0, 2]]).T
area_between_two_curves(p, q)  # Output: 0.0
area_between_two_curves(q, p)  # Output: 1.0

This change can be summarized as the old code would rearrange the formation of a natural triangle of four points. The rearrangement of four points fundamentally changes the shape, and therefore the area within those four points. The new code thanks to @mcnick now allows for the shoelace area to be calculated for a natural triangle of four points (i.e. a triangle where two of the four points are the same).

@cjekel cjekel merged commit 765d34c into cjekel:master Oct 7, 2023
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.

None yet

2 participants