-
Notifications
You must be signed in to change notification settings - Fork 12
Umbrella (tip) descriptor oriented #345
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
Umbrella (tip) descriptor oriented #345
Conversation
…e operations into existing umbrella descriptor functions with optional checkOrientation arg
|
The tests in the /tst directory are generated automatically by the documentation, i.e. the corresponding example in the documentation has to be changed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #345 +/- ##
==========================================
+ Coverage 70.58% 70.76% +0.17%
==========================================
Files 62 62
Lines 17269 17403 +134
==========================================
+ Hits 12190 12315 +125
- Misses 5079 5088 +9
🚀 New features to boost your workflow:
|
MeikeWeiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, thank you!!
Is it possible to combine some parts of the normal version and the tip version in a common method?
What happens if the vertices of the given surface are not dense, like for example here:
SimplicialSurfaceByUmbrellaDescriptor([(1,2,3),(1,2,4),(1,3,4),,(2,3,4)])? Does the method work for these surfaces?
Oh i never thought of that. The next commit includes changes that handles this particular case, thanks for pointing out! |
…ptor operations and shortened these functions
MeikeWeiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few more comments, which also apply to the UmbrellaTipDescriptor.
MeikeWeiss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Thank you!
Includes an extension of the already existing operations Umbrella(Tip)DescriptorOfSurface by returning the umbrella descriptors with a common orientation in case the surface is orientable and the checkOrientation argument is true (optional arg, default is true). If checkOrientation is false, the operations skip the orientation process.
Note that since this modifies default behaviour of the mentioned operations on orientable simplicial surface i also had to adjust the examples in the documentation and tst/simplicialsurfaces03.tst line 285 to passing false for checkOrientation:
gap> ud := UmbrellaDescriptorOfSurface(surf, false);
.tst files in /tst are ignored, otherwise i would have commited that change as well.