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

Remove redundancy of triangulation #329

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Conversation

lxvm
Copy link
Contributor

@lxvm lxvm commented Dec 30, 2023

Hi,

This pr is attempting to fix #285 and #249 by removing redundant simplices produced by triangulation_indices. So far I have added 7 polyhedra to the tests and have checked whether their volume is calculated correctly. Of these, 5 volumes were previously calculated incorrectly, including several examples from the issues and the isocahedron, and two were correct, namely the 5-simplex and the dodecahedron.

The fix I have attempted so far is naively calling unique on the list of simplices, and with a bit more work I hope that I can correct the implementation itself. Since the implementation is somewhat different from the Cohen-Hickey algorithm I am still figuring this out.

@blegat
Copy link
Member

blegat commented Jan 30, 2024

Not sure why the tests are not running since I switched it to ready for review

@blegat blegat closed this Jan 30, 2024
@blegat blegat reopened this Jan 30, 2024
@blegat
Copy link
Member

blegat commented Jan 30, 2024

Can you rebase on top of the master branch ?

@lxvm
Copy link
Contributor Author

lxvm commented Jan 31, 2024

@blegat I rebased this pr onto master and fixed a missing test dependency

@blegat
Copy link
Member

blegat commented Jan 31, 2024

The tests don't work if you use the default library instead of CDDLib ?

test/volume.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Member

blegat commented Jan 31, 2024

Since the implementation is somewhat different from the Cohen-Hickey algorithm I am still figuring this out.

Yes I don't remember why I implemented like this. We could probably merge this PR with the new tests and fixed implementation using unique. Then, in a later PR, we could rewrite the implementation with Cohen-Hickey and see if we can remove the unique and the tests are still passing.

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (268c701) 88.95% compared to head (a867113) 88.95%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #329   +/-   ##
=======================================
  Coverage   88.95%   88.95%           
=======================================
  Files          37       37           
  Lines        3170     3170           
=======================================
  Hits         2820     2820           
  Misses        350      350           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lxvm
Copy link
Contributor Author

lxvm commented Jan 31, 2024

I updated the tests as you suggested and exchanged CDDLib for the default library in exact arithmetic.

I agree it would be great to merge this "patch" fix and revisit the algorithm. When I read the implementation, it is very similar to Cohen-Hickey in that it is recursive, however it does not perform certain manipulations such as elimination that I would expect from Cohen-Hickey. I think this is interesting and may be more efficient, i.e. fewer computations, but it would be useful to pinpoint where the redundancy originates from.

@lxvm lxvm changed the title [WIP] Remove redundancy of triangulation Remove redundancy of triangulation Jan 31, 2024
@blegat
Copy link
Member

blegat commented Feb 1, 2024

Yes, I don't remember exactly why I thought it would work but I could try to recollect my thoughts (good example of why comments are important ^^)

@blegat blegat merged commit 9ce0504 into JuliaPolyhedra:master Feb 1, 2024
6 checks passed
@lxvm lxvm deleted the issue285 branch February 1, 2024 13:21
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.

Possible bug in volume
2 participants