-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add some documentation for Regular_triangulation::insert_if_in_star #5370
Add some documentation for Regular_triangulation::insert_if_in_star #5370
Conversation
Would it be possible to get a different reviewer? While I did talk with Clément about some details at the time, I am not that familiar with this package, which was written by Samuel, Olivier and Clément (@cjamin also wrote the code in gudhi using those undocumented interfaces). I think Mael knows this code much better than I do... Actually, anyone familiar with the design of the 2D/3D code might be a suitable reviewer, to decide what should be a public interface or not. |
Sorry I thought you were using this code. No pb, I removed you. |
Sorry for the delay, @VincentRouvreau. Why do you think this should be documented? The function does not have an equivalent in
One could argue point # 1 is fine. Point # 2 is a little surprising: the conflict zone is empty, so |
The motivation for documenting it is that we are using it in Gudhi, and it is always a bit dangerous to depend on undocumented interfaces. This function is used in https://github.com/GUDHI/gudhi-devel/blob/c838e3ec441109cc02ea4612dd2189860662298f/src/Tangential_complex/include/gudhi/Tangential_complex.h#L1027 in the function |
Looking at point 3, if we reinsert a point with exactly the same weight (why not ≤?), we don't insert, and then only if the dimension is 0 (so p is the same as star_center) we replace the only vertex with the new point (strange that is doesn't have an early out, it falls through to insert_in_conflicting_cell). In other cases it goes to insert_in_conflicting_cell which I think does check the conflict zone. No? |
Right, I skimmed a little too fast over the dimension check. Point #2 could be just:
but maybe it is not worth the behavior break? |
Yes, that change makes sense to me. For any behavior change we will have to test a bit in gudhi, but I don't see how this one could affect us. |
It is also unused in CGAL and the probability some third party is using that very specific undocumented function is relatively low.
Sure. There are just some sentences that I would rephrase in the doc, I will put it directly in suggestion in review of the PR. |
I added your proposal on e511b68 and tested it. It seems it does not break our testing suites, neither some custom tests I performed. |
Co-authored-by: Mael <mael.rouxel.labbe@geometryfactory.com>
Co-authored-by: Mael <mael.rouxel.labbe@geometryfactory.com>
/build:v0 |
The documentation is built. It will be available, after a few minutes, here : https://cgal.github.io/5370/v0/Manual/index.html |
@VincentRouvreau can you remove the trailing whitespace reported in the check |
Co-authored-by: Mael <mael.rouxel.labbe@geometryfactory.com>
Successfully tested in https://cgal.geometryfactory.com/CGAL/testsuite/results-5.3-Ic-78.shtml |
The small feature corresponding to this PR is accepted. It has a modification to |
|
Please use the following template to help us managing pull requests.
Summary of Changes
There was no issue in the tracker, but
Regular_triangulation::insert_if_in_star
was not documented.Release Management