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

PMP Corefinement hard-codes the use of Epeck in various places. #5119

Open
pentacular opened this issue Oct 26, 2020 · 8 comments
Open

PMP Corefinement hard-codes the use of Epeck in various places. #5119

pentacular opened this issue Oct 26, 2020 · 8 comments
Labels

Comments

@pentacular
Copy link
Contributor

Issue Details

PMP Corefinement hard-codes the use of Epeck in various places.

This makes it impossible to use with environments that do not support Epeck.

I was going to try to wire through the template parameters so that the a kernel could be configured, but I thought I'd better ask first in case there's some interesting reason for this hard-coding.

Is there any reason to keep the hard coding? :)

Source Code

typedef CGAL::Exact_predicates_exact_constructions_kernel EK;

Environment

  • CGAL version: 5.1
@pentacular pentacular changed the title PMP Corefinement hard-codes the use of Epeck in various places. PMP Corefinement hard-codes the use of Epeck in various places. label:Pkg::PMP Oct 26, 2020
@pentacular pentacular changed the title PMP Corefinement hard-codes the use of Epeck in various places. label:Pkg::PMP PMP Corefinement hard-codes the use of Epeck in various places. Oct 26, 2020
@sloriot
Copy link
Member

sloriot commented Oct 26, 2020

What environment are you using?

@pentacular
Copy link
Contributor Author

pentacular commented Oct 26, 2020

This issue isn't environment dependent.

However, the reason that I noticed it is that I am using wasm, which means that Epeck doesn't work, but kernels like Simple_cartesian<Gmpq> seem to.

@sloriot
Copy link
Member

sloriot commented Oct 27, 2020

The use of EPECK is here for robustness issues. In practice I could get rid of it but this requires a major effort. Why Epeck does not work? Are you simply having trouble with rounding mode changes and is the issue (also) related to gmp?

@pentacular
Copy link
Contributor Author

pentacular commented Oct 27, 2020

Yes, I believe it's due to the lack of rounding mode change support in wasm.

gmp seems to work because it does not rely on rounding modes.

What I was thinking is that it might be relatively simple (if tedious) to wire an Exact_kernel template parameter through to these Epeck and allow a user to supply an alternate kernel to use for these robust operations.

Defaulting this parameter to Epeck would preserve current behavior.

Does that sound like a workable approach?

@sloriot
Copy link
Member

sloriot commented Oct 27, 2020

The pb is that without interval you have no filter at all so even if we replace the internal EPECK by Simple_cartesian<Gmpq> we also need to change the input kernel to Simple_cartesian<Gmpq> but then I'm afraid the runtime will be a killer for your application. I have an idea to make the Interval work without changing the rounding mode but I cannot tell you when I'll have time to try it.

@pentacular
Copy link
Contributor Author

That would be great -- I was wondering if there would be a reasonable way to get a slightly slower Epeck happening without rounding magic.

I understand that there may be performance issues, but at least it should be testable.
In any case, you'd still want to be able to parameterize the kernel to take in a kernel with modified interval.

So it sounds like making the internal exact kernel parameterizable is the right thing to do regardless, and there are no real downsides to it.

If so, I'll give it a go in a bit.

@sloriot
Copy link
Member

sloriot commented Nov 12, 2020

Thinking about it, what you want is simply changing what
CGAL::Exact_predicates_exact_constructions means on a system without intervals.

So editing Kernel_23/include/CGAL/Exact_predicates_exact_constructions_kernel.h and have something like

#ifdef CGAL_HAS_NO_INTERVAL_SUPPORT
#define CGAL::Simple_cartesian<Epeck_ft> Epeck;
#else
... current code here ...
#endif

should be sufficient.

I don't think there is an automatic way to detect when CGAL_HAS_NO_INTERVAL_SUPPORT should be defined by you can do it by hand in your project.

@pentacular
Copy link
Contributor Author

pentacular commented Nov 16, 2020

Ok, this seems to be more or less working given:

class Epeck : public Type_equality_wrapper<Simple_cartesian<Epeck_ft>::Base<Epeck>::Type, Epeck> {};

Although that also needed a little patch in Polygon_mesh_processing/internal/Corefinement/intersection_nodes.h to work without a lazy kernel.

This was enough to get corefinement working.

I'll put a PR together in a couple of days and see how it goes.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants