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

integrals/intpoly : Add Mathematica test cases for 3D polytopes #13187

Merged
merged 2 commits into from
Aug 25, 2017

Conversation

ArifAhmed1995
Copy link
Contributor

@ArifAhmed1995 ArifAhmed1995 commented Aug 24, 2017

@certik
I have added the tests for octahedron, Great Stellated Dodecahedron and Echidnahedron.

Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good, thank you @ArifAhmed1995. Do the answers agree with @ndotsu's poster? I just pointed out some minor things, once those are fixed, this is ready to be merged.

[14, 16, 6, 9, 18], [19, 8, 7, 17, 15]]
# Actual volume is : 0.163118960624632
assert polytope_integrate(great_stellated_dodecahedron, 1) -\
0.163118960624632 < 1e-15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to use the abs(x-y) < eps idiom, not x-y < eps. The latter would be satisfied, e.g., for x=1, y=5.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15 digits are printed, but I would still lower eps to something like 1e-12 or so, just in case.

[14, 68, 71], [73, 91, 72], [13, 43, 64], [70, 71, 89],
[16, 72, 69], [44, 89, 62], [30, 69, 68], [45, 64, 91]]
# Actual volume is : 51.405764746872634
assert polytope_integrate(echidnahedron, 1) - 51.4057647468726 < 1e-15
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use abs(x-y)<eps. Also, only 13 decimal digits are printed, so you must use something like 1e-12 or so. You can't do 1e-15, because that might fail sometimes.

@ndotsu
Copy link

ndotsu commented Aug 25, 2017 via email

@ArifAhmed1995 ArifAhmed1995 changed the title Add Mathematica test cases for 3D polytopes integrals/intpoly : Add Mathematica test cases for 3D polytopes Aug 25, 2017
@ArifAhmed1995
Copy link
Contributor Author

@certik @ndotsu I have added the poster tests as well.

@certik certik merged commit b3e01a9 into sympy:master Aug 25, 2017
@ArifAhmed1995 ArifAhmed1995 deleted the intpoly3d_mathematica_tests branch August 25, 2017 15:08
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.

3 participants