New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Rational parsing in Mathematica parser #25717
Conversation
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
It appears that this is your initial pull request, so you'll need to include your name and email address in the |
sympy/parsing/mathematica.py
Outdated
@@ -975,6 +976,7 @@ def converter(expr): | |||
"Times": Mul, | |||
"Plus": Add, | |||
"Power": Pow, | |||
"Rational": lambda *a: Rational(*a), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
"Rational": lambda *a: Rational(*a), | |
"Rational": Rational, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason except for clarity that it's a two-argument function that takes its arguments in the same order as Mathematica. Probably better without the lambda-function, so I'll make that change.
Head branch was pushed to by a user without write access
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) | Change | Before [8059df73] <sympy-1.12^0> | After [c0c175e3] | Ratio | Benchmark (Parameter) |
|----------|------------------------------------|---------------------|---------|----------------------------------------------------------------------|
| - | 85.7±0.5ms | 57.1±0.4ms | 0.67 | integrate.TimeIntegrationRisch02.time_doit(10) |
| - | 85.2±1ms | 56.7±0.4ms | 0.67 | integrate.TimeIntegrationRisch02.time_doit_risch(10) |
| + | 22.4±0.06μs | 38.9±0.1μs | 1.74 | integrate.TimeIntegrationRisch03.time_doit(1) |
| - | 6.95±0.05ms | 3.75±0.01ms | 0.54 | logic.LogicSuite.time_load_file |
| - | 2.10±0ms | 660±1μs | 0.31 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 10.3±0.02ms | 1.96±0ms | 0.19 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse') |
| - | 369±0.8μs | 82.4±0.3μs | 0.22 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 4.87±0.01ms | 365±0.7μs | 0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 10.5±0.04ms | 1.09±0ms | 0.1 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse') |
| - | 6.28±0.01ms | 3.95±0.01ms | 0.63 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| - | 27.2±0.06ms | 12.1±0.03ms | 0.44 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 6.92±0.01ms | 1.17±0.01ms | 0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 16.2±0.03ms | 9.19±0.02ms | 0.57 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse') |
| - | 211±0.2ms | 70.7±0.05ms | 0.33 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 6.39±0.01ms | 534±1μs | 0.08 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse') |
| - | 27.8±0.05ms | 853±2μs | 0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse') |
| - | 612±1μs | 198±0.6μs | 0.32 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| - | 6.52±0.01ms | 202±1μs | 0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| - | 17.2±0.04ms | 205±1μs | 0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| - | 163±0.2μs | 89.4±0.2μs | 0.55 | solve.TimeMatrixOperations.time_rref(3, 0) |
| - | 312±0.4μs | 108±0.4μs | 0.35 | solve.TimeMatrixOperations.time_rref(4, 0) |
| - | 32.2±0.2ms | 13.5±0.03ms | 0.42 | solve.TimeSolveLinSys189x49.time_solve_lin_sys |
Full benchmark results can be found as artifacts in GitHub Actions |
References to other Issues or PRs
Fixes #25716
Brief description of what is fixed or changed
The Mathematica parser at present creates an
AppliedUndef
function with the same name and syntax as the built-in sympyRational
type, but none of the properties. This is because the conversion is not defined in the parser.This PR adds the relevant conversion and a simple test to the test cases.
Release Notes