Skip to content

Conversation

@cserteGT3
Copy link
Contributor

@cserteGT3 cserteGT3 commented Nov 3, 2025

This PR implements a specialized version for isconvex for Pyramid (as discussed in #1265). It assumes that the first four vertices construct the base.

Fix #1265

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.91%. Comparing base (7c2fbc3) to head (28e329a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1266   +/-   ##
=======================================
  Coverage   87.90%   87.91%           
=======================================
  Files         197      197           
  Lines        6240     6245    +5     
=======================================
+ Hits         5485     5490    +5     
  Misses        755      755           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Thank you @cserteGT3! Should we export the new base function?

@cserteGT3
Copy link
Contributor Author

Thank you @cserteGT3! Should we export the new base function?

It's already exported. I chose the base name as it is used by Cone.

@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

Could you please add some tests for the new base of Pyramid? That should complete the PR 💯

Copy link
Member

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

Some remarks/comments below.

@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

Tests are failing due to some unrelated issue with DataStructures.jl 🍇

@cserteGT3
Copy link
Contributor Author

I've did similar updates, just added a comment on convexity and fixed a bug I introduced (using variable p instead of pyramid).

@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

Added a new apex method for consistency. Now Pyramid has base and apex and both functions are used in the pyramid(u,v,w) implementation.

@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

@cserteGT3 could you please add some tests for the base and apex? Other than that, I think the PR is ready ✅

@JoshuaLampert
Copy link
Member

@cserteGT3 could you please add some tests for the base and apex? Other than that, I think the PR is ready ✅

Apart from running the formatter.

@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

Just added the test on GitHub directly 👍🏽

@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

Apart from running the formatter.

Good point! @cserteGT3 you can press Ctrl+Shift+I to get the formatting right.

@cserteGT3
Copy link
Contributor Author

Apart from running the formatter.

Good point! @cserteGT3 you can press Ctrl+Shift+I to get the formatting right.

Thank you both for shaping this PR! Ran the tests locally, all passed.

Regarding the formatting: I don't see that it would be different, than other files. Running "Format document" in VSCode doesn't change anything for me in pyramid.jl, though I think it is not configured properly, so I have to look into it...

@JoshuaLampert
Copy link
Member

Apart from running the formatter.

Good point! @cserteGT3 you can press Ctrl+Shift+I to get the formatting right.

Thank you both for shaping this PR! Ran the tests locally, all passed.

Regarding the formatting: I don't see that it would be different, than other files. Running "Format document" in VSCode doesn't change anything for me in pyramid.jl, though I think it is not configured properly, so I have to look into it...

test/predicates.jl is not formatted correctly, see https://github.com/JuliaGeometry/Meshes.jl/actions/runs/19064950241/job/54453184291?pr=1266#step:2:572.

@cserteGT3
Copy link
Contributor Author

Apart from running the formatter.

Good point! @cserteGT3 you can press Ctrl+Shift+I to get the formatting right.

Thank you both for shaping this PR! Ran the tests locally, all passed.
Regarding the formatting: I don't see that it would be different, than other files. Running "Format document" in VSCode doesn't change anything for me in pyramid.jl, though I think it is not configured properly, so I have to look into it...

test/predicates.jl is not formatted correctly, see https://github.com/JuliaGeometry/Meshes.jl/actions/runs/19064950241/job/54453184291?pr=1266#step:2:572.

Thanks for the pointer! Figured out my VSCode setting and pushed the change.

@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

Opened an issue in StatsBase.jl, which is calling DataStructures.jl: JuliaStats/StatsBase.jl#974 I believe our only use of StatsBase.jl is for weighted sampling. We should seek alternatives to drop this dependency once and for all.

Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

Restarted the actions to see if they pass now. Other than that, the PR looks good to me.

@juliohm juliohm merged commit 0983e44 into JuliaGeometry:master Nov 4, 2025
16 of 31 checks passed
@juliohm
Copy link
Member

juliohm commented Nov 4, 2025

Thank you @cserteGT3 ! Another one for the list 💯 🙂

@cserteGT3
Copy link
Contributor Author

Thank you for the quick merge and the collaborative efforts!

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.

Missing isconvex method for Pyramid

3 participants