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: sage-9.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:

Status badges

Description (last modified by rws)

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 rws

  • Description modified (diff)

comment:2 Changed 4 years ago by rws

  • Branch set to u/rws/improve_conversion_of_inexact_symbolic_values

comment:3 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 66b1e8764b35c59a47f490acdf0ca7e36b721fc6
  • Status changed from new to needs_review

New commits:

66b1e8724317: Improve conversion of inexact symbolic values

comment:4 Changed 4 years ago by git

  • Commit changed from 66b1e8764b35c59a47f490acdf0ca7e36b721fc6 to 83af67e38b1fc99334735ff3767e28696110ead8

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

83af67e24317: one more failsafe

comment:5 follow-up: Changed 4 years ago by 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).

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by 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. Moreover this can handle cases that are unknown at the moment.

comment:7 Changed 4 years ago by rws

Your conversion code (when EVER it exists) will be called automatically BTW.

comment:8 in reply to: ↑ 6 Changed 4 years ago by vdelecroix

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 vdelecroix

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 rws

  • 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 rws

  • Authors Ralf Stephan deleted
  • 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 vdelecroix

#24318 gets merged in 8.2.beta0

comment:13 Changed 4 years ago by rws

  • Branch set to u/rws/24317

comment:14 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • 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:

1bac7a624317: Doctest: Improve conversion of inexact symbolic values

comment:15 Changed 12 months ago by gh-DaveWitteMorris

  • Milestone changed from sage-8.2 to sage-9.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 vbraun

  • Branch changed from u/rws/24317 to 1bac7a61ad85af25aa4d9011f4b86a000c5039da
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.