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

Regarding code update in "is_simple_quad" function on Aug 18,2019 #7

Closed
aanchalMongia opened this issue Dec 20, 2019 · 5 comments
Closed

Comments

@aanchalMongia
Copy link

Dear Authors,

Thanks for your contribution in the form of "simialritymeasures" library for quantifying the difference between the curves. I have been using it for finding the area between the curves.
But, since your update in the code to check if the quadrilateral is simple or not [ in "is_simple_quad" function on Aug 18,2019], the output for area between the curves is not correct. (However, if I use the previous code the area returned is correct).
Specifically, the "if condition" which checks the number of cross products with same sign, should be:
sum(crossTF) > 2
instead of
sum(crossTF) == 2

The same can be checked from the following code which tries to find the area between two simple curves. Running the following prints :
area1 : 0.0

while using the previous code give correct area (4 in this case)

import matplotlib.pyplot as plt
import similaritymeasures

xaxis=[0,1, 2, 3, 4]
curve1=[0,0,0,0,0]
curve2=[1,1,1,1,1]
exp_data = np.zeros((len(xaxis), 2))
num_data = np.zeros((len(xaxis), 2))

exp_data[:, 0] = xaxis
exp_data[:, 1] = curve1
num_data[:, 0] = xaxis
num_data[:, 1] = curve2

plt.figure()
plt.scatter(xaxis, curve1)
plt.scatter(xaxis, curve2)
plt.show()
        
area1=similaritymeasures.area_between_two_curves(exp_data, num_data)
print("area1 : "+str(area1) )```
@cjekel
Copy link
Owner

cjekel commented Dec 20, 2019

Thanks for creating this issue. Let me dive in and see what happened. The questionable commit is this one 09112e2

cjekel added a commit that referenced this issue Dec 20, 2019
@cjekel
Copy link
Owner

cjekel commented Dec 20, 2019

Thanks again for creating this issue with an easy example to reproduce.

I've changed the code in 27876eb to:

    if sum(crossTF) == 2:
        return True
    elif sum(crossTF) == 4:
        return True
    else:
        return False

Simple quadrilaterals can have two positive cross products or four positive cross products. Otherwise, it's a complex quadrilateral. There was a bug in my initial code, and the Aug 18 fix only created a new bug. Hopefully it's working now.

I've added your test case to the set of tests fdb0e6b .

I'll push a new version 0.4.1 to pypi shortly.

@aanchalMongia
Copy link
Author

Thanks for looking into the issue.

However, I'd like to quote the criteria for checking if the quadrilateral is simple or not which is mentioned in the paper the code is referring to ("Jekel, C. F., Venter, G., Venter, M. P., Stander, N., & Haftka, R. T. (2018). Similarity measures for identifying material parameters from hysteresis loops using inverse analysis. International Journal of Material Forming. https://doi.org/10.1007/s12289-018-1421-8"):

"A complex quadrilateral exists if and only if two of the above cross products are negative and the other two are positive. A simple quadrilateral will have at least three of the same sign cross products"

@cjekel
Copy link
Owner

cjekel commented Dec 21, 2019

Thanks again. I figured out what was throwing me off. The test_complex_quad I wrote wasn't written using vectors, but rather vertices. Thus the cross products didn't make any sense. I've fixed this test, and also added a test_simple_quad test.

The code is now back to using if sum(crossTF) > 2:. bbb8693

Thanks for raising this issue! I don't think I would have found this. I'll push a new 0.4.2 version to pypi shortly.

@aanchalMongia
Copy link
Author

You're welcome!

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

No branches or pull requests

2 participants