Opened 4 years ago
Closed 12 months ago
#24317 closed enhancement (fixed)
Doctest: Improve conversion of inexact symbolic values
Reported by:  rws  Owned by:  

Priority:  minor  Milestone:  sage9.3 
Component:  symbolics  Keywords:  
Cc:  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Dave Morris 
Report Upstream:  N/A  Work issues:  
Branch:  1bac7a6 (Commits, GitHub, GitLab)  Commit:  1bac7a61ad85af25aa4d9011f4b86a000c5039da 
Dependencies:  #24318  Stopgaps: 
Description (last modified by )
Because there is no conversion implemented between some fields this ticket adds fallback code that goes through a RR/CC intermediate. Example:
sage: SR(CBF(1+I))._convert({'parent':CDF}) ... AttributeError: 'sage.rings.complex_double.ComplexDoubleField_class' object has no attribute 'complex_field'
Change History (16)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 4 years ago by
 Branch set to u/rws/improve_conversion_of_inexact_symbolic_values
comment:3 Changed 4 years ago by
 Commit set to 66b1e8764b35c59a47f490acdf0ca7e36b721fc6
 Status changed from new to needs_review
comment:4 Changed 4 years ago by
 Commit changed from 66b1e8764b35c59a47f490acdf0ca7e36b721fc6 to 83af67e38b1fc99334735ff3767e28696110ead8
Branch pushed to git repo; I updated commit sha1. New commits:
83af67e  24317: one more failsafe

comment:5 followup: ↓ 6 Changed 4 years ago by
It is not the role of the symbolic ring to decide which conversion should exist. Concerning the example provided, the only reasonable way to go is to improve CBF
(see #24318).
comment:6 in reply to: ↑ 5 ; followup: ↓ 8 Changed 4 years ago by
Replying to vdelecroix:
It is not the role of the symbolic ring to decide which conversion should exist. Concerning the example provided, the only reasonable way to go is to improve
CBF
(see #24318).
Ah the symbolic ring does not decide that but the symbolic function code has to return the input type, and as long as this can be done with a few lines of code successfully it's certainly better to do so. Moreover this can handle cases that are unknown at the moment.
comment:7 Changed 4 years ago by
Your conversion code (when EVER it exists) will be called automatically BTW.
comment:8 in reply to: ↑ 6 Changed 4 years ago by
Replying to rws:
Replying to vdelecroix:
It is not the role of the symbolic ring to decide which conversion should exist. Concerning the example provided, the only reasonable way to go is to improve
CBF
(see #24318).Ah the symbolic ring does not decide that but the symbolic function code has to return the input type, and as long as this can be done with a few lines of code successfully it's certainly better to do so.
You are pretending that it is successful. Conversion is not a trivial task. Going through an intermediate ring is not innocent (especially with floating point).
Moreover this can handle cases that are unknown at the moment.
For me it is: this proposal is hiding missing conversions that will remain unknown.
Again: the aim of the symbolic ring is not to be better than the rest of Sage. If a conversion does not exist, then it does not exist. It might be a bug, a mistake, but it is not the role of the symbolic ring to complete the conversion.
comment:9 Changed 4 years ago by
At least, the doctest mentioned in the ticket description should be solved by #24318 that is in needs review.
comment:10 Changed 4 years ago by
 Dependencies set to #24318
Okay, I tested #24318 and it works. This ticket should then add these doctests to ex._convert
:
sage: SR(RBF(1))._convert({'parent':RDF}) 1.0 sage: SR(CBF(1))._convert({'parent':RDF}) 1.0 sage: type(_.pyobject()) <type 'sage.rings.real_double.RealDoubleElement'> sage: SR(CBF(1+I))._convert({'parent':RDF}) 1.0 + 1.0*I sage: type(_.pyobject()) <type 'sage.rings.complex_double.ComplexDoubleElement'> sage: SR(CBF(1+I))._convert({'parent':CDF}) 1.0 + 1.0*I
comment:11 Changed 4 years ago by
 Branch u/rws/improve_conversion_of_inexact_symbolic_values deleted
 Commit 83af67e38b1fc99334735ff3767e28696110ead8 deleted
 Status changed from needs_review to needs_work
comment:12 Changed 4 years ago by
#24318 gets merged in 8.2.beta0
comment:13 Changed 4 years ago by
 Branch set to u/rws/24317
comment:14 Changed 4 years ago by
 Commit set to 1bac7a61ad85af25aa4d9011f4b86a000c5039da
 Status changed from needs_work to needs_review
 Summary changed from Improve conversion of inexact symbolic values to Doctest: Improve conversion of inexact symbolic values
New commits:
1bac7a6  24317: Doctest: Improve conversion of inexact symbolic values

comment:15 Changed 12 months ago by
 Milestone changed from sage8.2 to sage9.3
 Priority changed from major to minor
 Reviewers set to Dave Morris
 Status changed from needs_review to positive_review
 Type changed from defect to enhancement
The PR just adds some doctests (and makes minor formatting corrections).
comment:16 Changed 12 months ago by
 Branch changed from u/rws/24317 to 1bac7a61ad85af25aa4d9011f4b86a000c5039da
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
24317: Improve conversion of inexact symbolic values