Opened 4 years ago

Closed 4 years ago

#20074 closed enhancement (fixed)

QQbar cleaning 2

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-7.1
Component: number fields Keywords:
Cc: Merged in:
Authors: Vincent Delecroix Reviewers: Frédéric Chapoton, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 5cc7c49 (Commits) Commit: 5cc7c4946ca5524e4caec8de0ae2e5acf201943f
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

We further simplify QQbar code by:

  • using python operator to indentify binary operators instead of strings. In other words we replace '+' by operator.add, '-' by operator.sub, etc
  • writing only one function binop instead of addsub and muldiv.
  • removing the methods kind, is_exact, is_rational, is_field_element of descriptors and instead use the class themselves
  • removing the method rational_value and directly access the _value attribute
  • detect unions earlier in the code to avoid constructing ANBinaryExpr

As a consequence of the last items, we have exactification detected earlier

sage: sqrt17 = QQbar(17).sqrt()
sage: sqrt19 = QQbar(19).sqrt()
sage: (sqrt17 + sqrt19).exactify()
sage: sqrt17 * sqrt19 + sqrt17 - sqrt19 * sqrt17 - sqrt17
0

Instead of 0.?e-17 on the current beta. See also the better output in doctests from the commit 7fa8ed4.

follow up: #19955

Change History (21)

comment:1 Changed 4 years ago by vdelecroix

  • Branch set to u/vdelecroix/20074
  • Commit set to 9264ff161e50900ce17ca62393d70b8abc4b4bb6
  • Description modified (diff)
  • Status changed from new to needs_review

Last 10 new commits:

b35d6f6Trac 19954: fix doctests
9f39b58Trac 19954: fix sage_input doctests
7ab3ffbTrac 19954: move the 34-gon as a doctest
ec2e9aaTrac 19954: merge 7.1.beta1
d1788a0Trac 19954: fix doctests
94629dbTrac 19954: doctest independent of execution order
cb2592cTrac 19954: typo in documentation
1385753Trac 19954: documentation
c1f5c5fTrac 19954: remove useless "gaussian" functions
9264ff1Trac 20074: qqbar cleaning

comment:2 Changed 4 years ago by git

  • Commit changed from 9264ff161e50900ce17ca62393d70b8abc4b4bb6 to 012ae1865e4c2ab7bdf422e58d4b806b69ebc069

Branch pushed to git repo; I updated commit sha1. New commits:

012ae18Trac 20074: fix doctests

comment:3 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:4 Changed 4 years ago by git

  • Commit changed from 012ae1865e4c2ab7bdf422e58d4b806b69ebc069 to 66b22b5e512916c0d3f6cfb32cfc1603bb6b1980

Branch pushed to git repo; I updated commit sha1. New commits:

ff5ecccTrac 20074: merge 7.1.beta3
c23b72bTrac 20074: fix doctest
66b22b5Trac 20074: simplify descriptors

comment:5 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:6 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:7 Changed 4 years ago by vdelecroix

  • Dependencies #19954 deleted
  • Status changed from needs_work to needs_review

comment:8 Changed 4 years ago by git

  • Commit changed from 66b22b5e512916c0d3f6cfb32cfc1603bb6b1980 to 362a67b966045c2a89192df890a73e87e8354f84

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d716b88Trac 20074: qqbar cleaning
7fa8ed4Trac 20074: fix doctests
362a67bTrac 20074: simplify descriptors

comment:9 Changed 4 years ago by vdelecroix

  • Description modified (diff)

comment:10 follow-up: Changed 4 years ago by tscrim

For this change:

@@ -7461,14 +7076,16 @@ class ANBinaryExpr(ANDescr):
 
             op = self._op
 
-            if op == '+':
+            if op is operator.add:
                 value = left_value + right_value
-            if op == '-':
+            elif op is operator.sub:
                 value = left_value - right_value
-            if op == '*':
+            elif op is operator.mul:
                 value = left_value * right_value
-            if op == '/':
+            elif op is operator.div:
                 value = left_value / right_value
+            else:
+                raise RuntimeError("op is {}".format(op))
 
             if gen.is_trivial():
                 return ANRational(value)

Is there a reason why you did not just do value = op(left_value, right_value)?

Have you done any speed comparisons?

comment:11 in reply to: ↑ 10 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Replying to tscrim:

For this change: ... Is there a reason why you did not just do value = op(left_value, right_value)?

Nope.

Have you done any speed comparisons?

I don't think it changes anything. The code is just much simpler.

comment:12 Changed 4 years ago by git

  • Commit changed from 362a67b966045c2a89192df890a73e87e8354f84 to 563e3892794a39bf6cee4cd625cef58ae32ff1d8

Branch pushed to git repo; I updated commit sha1. New commits:

563e389Trac 20074: code simplification

comment:13 Changed 4 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:14 Changed 4 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, looks good to me. Patchbots are happy. Let it be.

comment:15 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, try the next beta...

comment:16 Changed 4 years ago by git

  • Commit changed from 563e3892794a39bf6cee4cd625cef58ae32ff1d8 to c379250155830789d0d1a5f163b6314068286beb

Branch pushed to git repo; I updated commit sha1. New commits:

c379250Trac 20074: merge 7.1.rc0

comment:17 Changed 4 years ago by tscrim

  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Travis Scrimshaw
  • Status changed from needs_work to positive_review

comment:18 Changed 4 years ago by vdelecroix

  • Status changed from positive_review to needs_work

I need to fix a doctest...

comment:19 Changed 4 years ago by git

  • Commit changed from c379250155830789d0d1a5f163b6314068286beb to 5cc7c4946ca5524e4caec8de0ae2e5acf201943f

Branch pushed to git repo; I updated commit sha1. New commits:

5cc7c49Trac 20074: fix doctest

comment:20 Changed 4 years ago by vdelecroix

  • Status changed from needs_work to positive_review

comment:21 Changed 4 years ago by vbraun

  • Branch changed from u/vdelecroix/20074 to 5cc7c4946ca5524e4caec8de0ae2e5acf201943f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.