Fix crash when specify invalid base for RR and RIF construction #39001
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Documentation preview for this PR (built with commit ff9d834; changes) is ready! 🎉 |
user202729
force-pushed
the
fix-real-construct-invalid-base
branch
from
e5bd6c7 to
91063db
Compare
5 tasks
user202729
force-pushed
the
fix-real-construct-invalid-base
branch
from
91063db to
ff9d834
Compare
|
Is it a test that fails or sagemath that crashes? I can't reproduce or a weird behavior on my side. |
|
Before the patch when you run the test with invalid base it will crash SageMath. @JosePisco e.g. → Maybe your libmpfr build has assertion disabled by default (?) not sure. Maybe the first 5 tests are not directly related to the patch, but might as well be careful meanwhile. |
JosePisco
approved these changes
Nov 20, 2024
tscrim
approved these changes
Nov 26, 2024
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 6, 2024
As in the title. It allows you to say e.g. `RIF("1.23?2e-5")`.
Partially handles sagemath#36797. (only
for real case. Complex case is not handled yet, but in principle it
should not be too difficult.)
(I don't see any disadvantage of allowing this, it's backwards
compatible)
Issue: currently
```
sage: RIF("10", base=37)
37
sage: ZZ("10", base=37)
[error]
```
should this inconsistency be fixed? If so how?
[Edit: actually the rule of conversion should probably follow [string to
mpfr conversion rule](https://www.mpfr.org/mpfr-current/mpfr.html#index-
mpfr_005fstrtofr) or [string to mpz conversion
rule](https://gmplib.org/manual/Assigning-Integers) ]
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (there's no documentation change, but should we explicitly
mention the feature? I think the feature to construct from `[a..b]`
isn't explicitly mentioned either…?)
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
sagemath#39001
URL: sagemath#38998
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 6, 2024
…construction
As in the title.
Note that currently `ZZ(str, base=int)` only support 2 to 36. Should
this be changed for consistency?
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (There's no change in documentation.)
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
------
The test on Conda (Mac) fails with
```
2024-11-19T04:02:36.0696760Z ./bootstrap: line 142: aclocal: command not
found
2024-11-19T04:02:36.0697900Z Bootstrap failed. Either install autotools;
or run bootstrap with
2024-11-19T04:02:36.0698610Z the -d option to download the auto-
generated files instead.
```
Looks unrelated.
Another one (Ubuntu 3.9) is
```
2024-11-19T04:26:04.0471925Z
**********************************************************************
2024-11-19T04:26:04.0591101Z File "src/sage/plot/plot.py", line 1857, in
sage.plot.plot.plot
2024-11-19T04:26:04.0591946Z Failed example:
2024-11-19T04:26:04.0592761Z plot(f, (x, -3.5, 3.5),
detect_poles='show', exclude=[-3..3],
2024-11-19T04:26:04.0593588Z ymin=-5, ymax=5)
2024-11-19T04:26:04.0594061Z Expected:
2024-11-19T04:26:04.0649808Z Graphics object consisting of 12
graphics primitives
2024-11-19T04:26:04.0650471Z Got:
2024-11-19T04:26:04.1009273Z Graphics object consisting of 13
graphics primitives
2024-11-19T04:26:26.6704957Z
**********************************************************************
```
Also looks unrelated. Reported in sagemath#39002.
URL: sagemath#39001
Reported by: user202729
Reviewer(s): grnx, Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 8, 2024
As in the title. It allows you to say e.g. `RIF("1.23?2e-5")`.
Partially handles sagemath#36797. (only
for real case. Complex case is not handled yet, but in principle it
should not be too difficult.)
(I don't see any disadvantage of allowing this, it's backwards
compatible)
Issue: currently
```
sage: RIF("10", base=37)
37
sage: ZZ("10", base=37)
[error]
```
should this inconsistency be fixed? If so how?
[Edit: actually the rule of conversion should probably follow [string to
mpfr conversion rule](https://www.mpfr.org/mpfr-current/mpfr.html#index-
mpfr_005fstrtofr) or [string to mpz conversion
rule](https://gmplib.org/manual/Assigning-Integers) ]
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (there's no documentation change, but should we explicitly
mention the feature? I think the feature to construct from `[a..b]`
isn't explicitly mentioned either…?)
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
sagemath#39001
URL: sagemath#38998
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 8, 2024
…construction
As in the title.
Note that currently `ZZ(str, base=int)` only support 2 to 36. Should
this be changed for consistency?
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (There's no change in documentation.)
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
------
The test on Conda (Mac) fails with
```
2024-11-19T04:02:36.0696760Z ./bootstrap: line 142: aclocal: command not
found
2024-11-19T04:02:36.0697900Z Bootstrap failed. Either install autotools;
or run bootstrap with
2024-11-19T04:02:36.0698610Z the -d option to download the auto-
generated files instead.
```
Looks unrelated.
Another one (Ubuntu 3.9) is
```
2024-11-19T04:26:04.0471925Z
**********************************************************************
2024-11-19T04:26:04.0591101Z File "src/sage/plot/plot.py", line 1857, in
sage.plot.plot.plot
2024-11-19T04:26:04.0591946Z Failed example:
2024-11-19T04:26:04.0592761Z plot(f, (x, -3.5, 3.5),
detect_poles='show', exclude=[-3..3],
2024-11-19T04:26:04.0593588Z ymin=-5, ymax=5)
2024-11-19T04:26:04.0594061Z Expected:
2024-11-19T04:26:04.0649808Z Graphics object consisting of 12
graphics primitives
2024-11-19T04:26:04.0650471Z Got:
2024-11-19T04:26:04.1009273Z Graphics object consisting of 13
graphics primitives
2024-11-19T04:26:26.6704957Z
**********************************************************************
```
Also looks unrelated. Reported in sagemath#39002.
URL: sagemath#39001
Reported by: user202729
Reviewer(s): grnx, Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 11, 2024
As in the title. It allows you to say e.g. `RIF("1.23?2e-5")`.
Partially handles sagemath#36797. (only
for real case. Complex case is not handled yet, but in principle it
should not be too difficult.)
(I don't see any disadvantage of allowing this, it's backwards
compatible)
Issue: currently
```
sage: RIF("10", base=37)
37
sage: ZZ("10", base=37)
[error]
```
should this inconsistency be fixed? If so how?
[Edit: actually the rule of conversion should probably follow [string to
mpfr conversion rule](https://www.mpfr.org/mpfr-current/mpfr.html#index-
mpfr_005fstrtofr) or [string to mpz conversion
rule](https://gmplib.org/manual/Assigning-Integers) ]
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (there's no documentation change, but should we explicitly
mention the feature? I think the feature to construct from `[a..b]`
isn't explicitly mentioned either…?)
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
sagemath#39001
URL: sagemath#38998
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 12, 2024
As in the title. It allows you to say e.g. `RIF("1.23?2e-5")`.
Partially handles sagemath#36797. (only
for real case. Complex case is not handled yet, but in principle it
should not be too difficult.)
(I don't see any disadvantage of allowing this, it's backwards
compatible)
Issue: currently
```
sage: RIF("10", base=37)
37
sage: ZZ("10", base=37)
[error]
```
should this inconsistency be fixed? If so how?
[Edit: actually the rule of conversion should probably follow [string to
mpfr conversion rule](https://www.mpfr.org/mpfr-current/mpfr.html#index-
mpfr_005fstrtofr) or [string to mpz conversion
rule](https://gmplib.org/manual/Assigning-Integers) ]
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (there's no documentation change, but should we explicitly
mention the feature? I think the feature to construct from `[a..b]`
isn't explicitly mentioned either…?)
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
sagemath#39001
URL: sagemath#38998
Reported by: user202729
Reviewer(s): Travis Scrimshaw
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Dec 13, 2024
As in the title. It allows you to say e.g. `RIF("1.23?2e-5")`.
Partially handles sagemath#36797. (only
for real case. Complex case is not handled yet, but in principle it
should not be too difficult.)
(I don't see any disadvantage of allowing this, it's backwards
compatible)
Issue: currently
```
sage: RIF("10", base=37)
37
sage: ZZ("10", base=37)
[error]
```
should this inconsistency be fixed? If so how?
[Edit: actually the rule of conversion should probably follow [string to
mpfr conversion rule](https://www.mpfr.org/mpfr-current/mpfr.html#index-
mpfr_005fstrtofr) or [string to mpz conversion
rule](https://gmplib.org/manual/Assigning-Integers) ]
### 📝 Checklist
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (there's no documentation change, but should we explicitly
mention the feature? I think the feature to construct from `[a..b]`
isn't explicitly mentioned either…?)
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
sagemath#39001
URL: sagemath#38998
Reported by: user202729
Reviewer(s): Travis Scrimshaw
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As in the title.
Note that currently
ZZ(str, base=int)only support 2 to 36. Should this be changed for consistency?📝 Checklist
⌛ Dependencies
The test on Conda (Mac) fails with
Looks unrelated.
Another one (Ubuntu 3.9) is
Also looks unrelated. Reported in #39002.