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

Core: Use Expr::is_zero() of AST #8002

Merged
merged 5 commits into from
Mar 14, 2024
Merged

Conversation

afabri
Copy link
Member

@afabri afabri commented Jan 25, 2024

Summary of Changes

Accelerate comparison with zero

Release Management

  • Affected package(s): Number_types
  • License and copyright ownership: unchanged

@mglisse
Copy link
Member

mglisse commented Jan 25, 2024

Is this implementation of Is_one really faster? From what I can see, doubleInterval itself should already be more costly than ==1.

@afabri
Copy link
Member Author

afabri commented Jan 26, 2024

When comparing to 1 that constructs an Expr(1), and performs a subtraction. I wanted to avoid that.
I am also wondering if one should not use Lazy_exact_nt<Expr> instead of doing a filtering, but I said myself that Expr is somehow the same, but didn't find the interval hardcoded.
And I am wondering if I first have to call Expr::approx() as done here

@mglisse
Copy link
Member

mglisse commented Jan 26, 2024

When comparing to 1 that constructs an Expr(1), and performs a subtraction. I wanted to avoid that.

double d = doubleValue();
if (! CGAL_CORE_finite(d)) { // if overflow, underflow or NaN
lb = ub = d;
return;
}
int sign = ((* this) -Expr(d)).sign();

doubleInterval also constructs an Expr from a double and performs a subtraction...

And I am wondering if I first have to call Expr::approx()

Looks like doubleValue already does it? Although the different precision is suspicious.

double doubleValue() const {
return approx(53, 1024).doubleValue();

but didn't find the interval hardcoded.

IIUC (but I know little about CORE) it stores an approximate value as a Real and a precision, not directly an interval.

@sloriot sloriot added Under Testing Batch_1 First Batch of PRs under testing Tested and removed Under Testing Ready to be tested Batch_1 First Batch of PRs under testing labels Feb 13, 2024
@sloriot
Copy link
Member

sloriot commented Feb 16, 2024

Successfully tested in CGA-6.0-Ic-173

@lrineau
Copy link
Member

lrineau commented Feb 16, 2024

@afabri @mglisse Can you please go on with the discussion? I think Andreas has to answer to Marc's last remarks.

@github-actions github-actions bot removed the Tested label Feb 22, 2024
Copy link

This pull-request was previously marked with the label Tested, but has been modified with new commits. That label has been removed.

@afabri
Copy link
Member Author

afabri commented Feb 22, 2024

For the time being let's reduce it to the initial scope, namely to have a faster test for being zero.

@sloriot
Copy link
Member

sloriot commented Mar 13, 2024

Successfully tested in CGAL-6.0-Ic-192

@lrineau lrineau self-assigned this Mar 14, 2024
@lrineau lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Mar 14, 2024
@lrineau lrineau merged commit 217bfa3 into CGAL:master Mar 14, 2024
9 checks passed
@lrineau lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Mar 14, 2024
@lrineau lrineau deleted the CORE-zero_one-GF branch March 14, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants