Opened 6 years ago
Closed 6 years ago
#20074 closed enhancement (fixed)
QQbar cleaning 2
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage7.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, GitHub, GitLab)  Commit:  5cc7c4946ca5524e4caec8de0ae2e5acf201943f 
Dependencies:  Stopgaps: 
Description (last modified by )
We further simplify QQbar code by:
 using python operator to indentify binary operators instead of strings. In other words we replace
'+'
byoperator.add
,''
byoperator.sub
, etc  writing only one function
binop
instead ofaddsub
andmuldiv
.  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.?e17
on the current beta. See also the better output in doctests from the commit 7fa8ed4
.
follow up: #19955
Change History (21)
comment:1 Changed 6 years ago by
 Branch set to u/vdelecroix/20074
 Commit set to 9264ff161e50900ce17ca62393d70b8abc4b4bb6
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit changed from 9264ff161e50900ce17ca62393d70b8abc4b4bb6 to 012ae1865e4c2ab7bdf422e58d4b806b69ebc069
Branch pushed to git repo; I updated commit sha1. New commits:
012ae18  Trac 20074: fix doctests

comment:3 Changed 6 years ago by
 Description modified (diff)
comment:4 Changed 6 years ago by
 Commit changed from 012ae1865e4c2ab7bdf422e58d4b806b69ebc069 to 66b22b5e512916c0d3f6cfb32cfc1603bb6b1980
comment:5 Changed 6 years ago by
 Description modified (diff)
comment:7 Changed 6 years ago by
 Dependencies #19954 deleted
 Status changed from needs_work to needs_review
comment:8 Changed 6 years ago by
 Commit changed from 66b22b5e512916c0d3f6cfb32cfc1603bb6b1980 to 362a67b966045c2a89192df890a73e87e8354f84
comment:9 Changed 6 years ago by
 Description modified (diff)
comment:10 followup: ↓ 11 Changed 6 years ago by
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 6 years ago by
 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 6 years ago by
 Commit changed from 362a67b966045c2a89192df890a73e87e8354f84 to 563e3892794a39bf6cee4cd625cef58ae32ff1d8
Branch pushed to git repo; I updated commit sha1. New commits:
563e389  Trac 20074: code simplification

comment:13 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:14 Changed 6 years ago by
 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 6 years ago by
 Status changed from positive_review to needs_work
Merge conflict, try the next beta...
comment:16 Changed 6 years ago by
 Commit changed from 563e3892794a39bf6cee4cd625cef58ae32ff1d8 to c379250155830789d0d1a5f163b6314068286beb
Branch pushed to git repo; I updated commit sha1. New commits:
c379250  Trac 20074: merge 7.1.rc0

comment:17 Changed 6 years ago by
 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 6 years ago by
 Status changed from positive_review to needs_work
I need to fix a doctest...
comment:19 Changed 6 years ago by
 Commit changed from c379250155830789d0d1a5f163b6314068286beb to 5cc7c4946ca5524e4caec8de0ae2e5acf201943f
Branch pushed to git repo; I updated commit sha1. New commits:
5cc7c49  Trac 20074: fix doctest

comment:20 Changed 6 years ago by
 Status changed from needs_work to positive_review
comment:21 Changed 6 years ago by
 Branch changed from u/vdelecroix/20074 to 5cc7c4946ca5524e4caec8de0ae2e5acf201943f
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
Trac 19954: fix doctests
Trac 19954: fix sage_input doctests
Trac 19954: move the 34gon as a doctest
Trac 19954: merge 7.1.beta1
Trac 19954: fix doctests
Trac 19954: doctest independent of execution order
Trac 19954: typo in documentation
Trac 19954: documentation
Trac 19954: remove useless "gaussian" functions
Trac 20074: qqbar cleaning