Skip to content

Re-add Epick_without_intervals #4306

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lrineau
Copy link
Member

@lrineau lrineau commented Oct 21, 2019

Summary of Changes

New attempt for Epick_without_intervals. First attempt was #3939, reverted by commit bb640d2 in #4096.

This time, I made it with the right kernel base, I think.

Release Management

  • Affected package(s): all

This time, I made it with the right kernel base, I think.
@lrineau lrineau added this to the 5.1-beta milestone Oct 21, 2019
@lrineau lrineau requested a review from MaelRL October 21, 2019 07:10
@lrineau lrineau self-assigned this Oct 21, 2019
@mglisse
Copy link
Member

mglisse commented Oct 21, 2019

Is the 'E' for "exact" true? It isn't clear where bignums appear in here.

@lrineau
Copy link
Member Author

lrineau commented Oct 21, 2019

You are right. I messed up again.

@lrineau lrineau added the Not yet approved The feature or pull-request has not yet been approved. label Oct 21, 2019
@mglisse
Copy link
Member

mglisse commented Oct 21, 2019

Mael had suggestions in #3939 (comment).

@MaelRL
Copy link
Member

MaelRL commented Oct 23, 2019

MaelRL@88ee987 is a proof of concept of what I meant. I only modified Orientation_3.

Running 'Kernel_23/test/Kernel_23/Cartesian' gives:

typeid k: N4CGAL5EpickE
static call in orientation_3
Calling base of static...
Calling a filtered predicate
Could not resolve with filtered predicate, calling base of filtered
typeid kiwi: N4CGAL23Epick_without_intervalsE
static call in orientation_3
Calling base of static...

@mglisse
Copy link
Member

mglisse commented Oct 23, 2019

MaelRL@88ee987 is a proof of concept of what I meant.

Makes sense, but you will need a (trivial) functor for all remaining predicates to replace them with an exact version, not just a fallback for those that have a static filter. An alternative would be to decompose it into 2 wrappers: one that replaces all predicates with their exact counterpart, and one that interposes static filters on known predicates, but this split is probably not necessary.

@mglisse
Copy link
Member

mglisse commented Oct 26, 2019

One simple way to get something like this is to modify Filtered_predicate and put everything but the last line of the body between #ifndef CGAL_NO_INTERVALS and #endif. Then you can just use Epick. (ok, probably this needs to be done in a few more places as well)

@MaelRL
Copy link
Member

MaelRL commented Feb 4, 2020

PR #4495 would also benefit from the same mechanism of Static_filters<CK> not automatically wrapping with Filtered_kernel_base. It uses a special type of filtering and cannot use CGAL::Filtered_predicate. For now, we have added (commit 46876c8) another class that omits this wrapping, but if changes from above were to be used it could be simply Static_filters<CK>.

@mglisse Since CGAL::Filtered_predicate is its own class, I don't know if we can handle everything with macros. Maybe just a template parameter to Filtered_kernel_base? But then wouldn't it look a bit weird with CGAL::Filtered_kernel_base<CK, DONT_FILTER>?

@mglisse
Copy link
Member

mglisse commented Feb 5, 2020

@mglisse Since CGAL::Filtered_predicate is its own class, I don't know if we can handle everything with macros.

The idea of a macro was first as a quick proof of concept, and second that the only point of Epick_without_interval (for users) is as a replacement for Epick on platforms where we cannot use intervals (no rounded operations), so it might as well be called Epick, controlled by some macros.

Maybe just a template parameter to Filtered_kernel_base? But then wouldn't it look a bit weird with CGAL::Filtered_kernel_base<CK, DONT_FILTER>?

You can call it CGAL::Filtered_kernel_base<CK, NO_INTERVAL> if that looks less weird. In software pipelines, I have seen a few times functions that take an argument enable=true/false, so that the pipeline is structurally constant, but the behavior can be controlled. So I don't find it shocking anymore...

(I haven't looked at this since October, so I don't remember much, and I haven't looked at your other branch either, so don't take my comment as something insightful that needs to be analyzed)

@MaelRL MaelRL changed the base branch from master to releases/CGAL-4.14-branch March 26, 2020 20:05
@MaelRL MaelRL changed the base branch from releases/CGAL-4.14-branch to master March 26, 2020 20:06
@maxGimeno
Copy link
Contributor

Is this PR ready for testing ?

@sloriot
Copy link
Member

sloriot commented Apr 10, 2020

Travis fails

@MaelRL
Copy link
Member

MaelRL commented Apr 10, 2020

The implementation is wrong. If I have time, I will enhance internal::Static_filters like I said above (I did part of it in the Filtered rational kernel), and it can be finished that way.

@MaelRL MaelRL modified the milestones: 5.1-beta, 5.2-beta Apr 14, 2020
@maxGimeno maxGimeno modified the milestones: 5.2-beta, 5.3-beta Oct 16, 2020
@sloriot sloriot modified the milestones: 5.3-beta, 5.4-beta Mar 22, 2021
@MaelRL MaelRL removed this from the 5.4-beta milestone Apr 9, 2021
@MaelRL MaelRL added this to the Trash / Attic milestone Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not yet approved The feature or pull-request has not yet been approved. Pkg::Kernel_23 Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants