Opened 4 months ago

Last modified 6 weeks ago

#29006 needs_review enhancement

Rational number constructor: handle leading '+'?

Reported by: jhpalmieri Owned by:
Priority: minor Milestone: sage-9.2
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Dave Morris Reviewers:
Report Upstream: N/A Work issues:
Branch: public/29006 (Commits) Commit: efaab6ff5af1a9e8e3c2bff818b535b316ce054e
Dependencies: Stopgaps:

Description

As reported on [ask.sagemath.org https://ask.sagemath.org/question/49537/unable-to-convert-string-to-rational-when-plus-sign-is-added/], we have the following:

sage: ZZ('+3')
3
sage: R.<x> = QQ[]
sage: R('+3')
3

but

sage: QQ('+3')
...
TypeError: unable to convert '+3' to a rational

I think this is because the gmp function mpq_set_str doesn't handle leading plus signs, but I'm not sure. In any case, should Sage handle this better?

Change History (5)

comment:1 Changed 4 months ago by gh-DaveWitteMorris

  • Branch set to public/29006

comment:2 Changed 4 months ago by gh-DaveWitteMorris

  • Authors set to Dave Morris
  • Commit set to efaab6ff5af1a9e8e3c2bff818b535b316ce054e
  • Status changed from new to needs_review

Your diagnosis is correct. The function mpq_set_str inherits this behavior from mpz_set_str.

I don't think it is a good idea for sagemath to go around telling people that it doesn't know how to turn '+3' and '+3/2' into rational numbers, so I wrote a (very easy) pull request to fix this. It patches mpz_set_str, which is the most direct solution, but I don't know whether there are practical considerations that make this undesirable.

I think the other obvious solution would be to preprocess the input to mpq_set_str, but that seems to me to be both inelegant and inefficient.

(Should the patch be sent upstream to mpir for their consideration? Perhaps it should go even farther upstream to gmp, but I don't know how to use mercurial. I'm not even very confident with git yet. I don't know how likely it is that either of these projects would accept the patch, since it's changing the behavior and not fixing a bug.)


New commits:

efaab6fallow leading plus

comment:3 Changed 4 months ago by gh-DaveWitteMorris

If fixing the problem is too much trouble, we could provide a more informative error message, such as TypeError: string '+3' is not formatted as a rational number -- leading '+' is not allowed. (I think complaining that the string is not properly formatted is more accurate than saying it cannot be converted.)

comment:4 Changed 4 months ago by jhpalmieri

This will only fix the problem for users who use Sage's mpir. I see this in config.log:

configure:22728: result:     mpir-3.0.0-644faf502c56f97d9accd301965fc57d6ec70868.p0 will not be installed (configure check)

so patching mpir won't be any help.

Submitting a patch to mpir would be a good idea, and if it's accepted, it will make its way to Sage, and also to various linux distributions, homebrew, etc. I guess this is the spot. I think there is a good chance they will accept the patch, and one of the main authors (Bill Hart) has been active in Sage development. So you can point to this ticket and it will have some resonance.

comment:5 Changed 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

Note: See TracTickets for help on using tickets.