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: |
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
- Branch set to public/29006
comment:2 Changed 15 months ago by
- Commit set to efaab6ff5af1a9e8e3c2bff818b535b316ce054e
- Status changed from new to needs_review
comment:3 Changed 15 months ago by
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
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
- 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
- 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
- Milestone changed from sage-9.2 to sage-9.3
comment:8 Changed 2 months ago by
- 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.
Your diagnosis is correct. The function
mpq_set_str
inherits this behavior frommpz_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 patchesmpz_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:
allow leading plus