Opened 15 months ago

Last modified 2 months ago

#29006 needs_work enhancement

Rational number constructor: handle leading '+'?

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

Status badges

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 (8)

comment:1 Changed 15 months ago by gh-DaveWitteMorris

  • Branch set to public/29006

comment:2 Changed 15 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 15 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 15 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 12 months 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.

comment:6 Changed 8 months ago by mkoeppe

  • Status changed from needs_review to needs_work

I agree that this would be a good improvement (in fact, more generally, to accept the same input strings as Python's Fraction class (https://docs.python.org/3/library/fractions.html).

But the implementation has to be done on a different level - in the sage library, not by patching an underlying library. We use a wide range of system installations of all libraries.

In fact, a possible solution could just to do try/except around the current code, falling back to Fraction and then extracting numerator and denominator.

comment:7 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:8 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

Note: See TracTickets for help on using tickets.