Skip to content
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

changes for fixing branch-cuts #25852

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Conversation

arnabnandikgp
Copy link
Contributor

References to other Issues or PRs

Fixes #23843

Brief description of what is fixed or changed

Fixed eval_aseries for atan and acot for fixing conflicting results at branch cuts

Release Notes

  • functions
    • fixed the eval_aseries method for atan and acot in order to ensure their correct working at the branch cuts.

@sympy-bot
Copy link

sympy-bot commented Oct 31, 2023

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:

  • functions
    • fixed the eval_aseries method for atan and acot in order to ensure their correct working at the branch cuts. (#25852 by @arnabnandikgp)

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.
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
Fixes #23843 

#### Brief description of what is fixed or changed
Fixed  `eval_aseries` for atan and acot for fixing conflicting results at branch cuts 

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:



NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->

* functions
  * fixed the eval_aseries method for atan and acot in order to ensure their correct working at the branch cuts.

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@anutosh491
Copy link
Member

anutosh491 commented Oct 31, 2023

Hey @arnabnandikgp I shall review the PR tomorrow and get back to you. Untill then could you please fix the authors check. The instructions for the same have been noted down here https://docs.sympy.org/dev/contributing/new-contributors-guide/workflow-process.html#mailmap-instructions

Copy link

github-actions bot commented Nov 1, 2023

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

| Change   | Before [a00718ba]    | After [39594aac]    |   Ratio | Benchmark (Parameter)                                                |
|----------|----------------------|---------------------|---------|----------------------------------------------------------------------|
| -        | 67.0±0.2ms           | 44.3±0.4ms          |    0.66 | integrate.TimeIntegrationRisch02.time_doit(10)                       |
| -        | 67.0±1ms             | 43.0±0.1ms          |    0.64 | integrate.TimeIntegrationRisch02.time_doit_risch(10)                 |
| +        | 17.9±0.2μs           | 28.9±0.2μs          |    1.62 | integrate.TimeIntegrationRisch03.time_doit(1)                        |
| -        | 5.34±0.05ms          | 2.87±0.03ms         |    0.54 | logic.LogicSuite.time_load_file                                      |
| -        | 72.3±0.4ms           | 28.3±0.09ms         |    0.39 | polys.TimeGCD_GaussInt.time_op(1, 'dense')                           |
| -        | 25.2±0.1ms           | 16.7±0.04ms         |    0.66 | polys.TimeGCD_GaussInt.time_op(1, 'expr')                            |
| -        | 71.6±0.6ms           | 28.5±0.1ms          |    0.4  | polys.TimeGCD_GaussInt.time_op(1, 'sparse')                          |
| -        | 251±1ms              | 124±0.3ms           |    0.49 | polys.TimeGCD_GaussInt.time_op(2, 'dense')                           |
| -        | 253±1ms              | 125±1ms             |    0.49 | polys.TimeGCD_GaussInt.time_op(2, 'sparse')                          |
| -        | 647±3ms              | 374±4ms             |    0.58 | polys.TimeGCD_GaussInt.time_op(3, 'dense')                           |
| -        | 644±5ms              | 369±1ms             |    0.57 | polys.TimeGCD_GaussInt.time_op(3, 'sparse')                          |
| -        | 493±2μs              | 286±2μs             |    0.58 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(1, 'dense')            |
| -        | 1.76±0.01ms          | 1.04±0ms            |    0.59 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(2, 'dense')            |
| -        | 5.67±0.02ms          | 3.02±0.01ms         |    0.53 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(3, 'dense')            |
| -        | 444±2μs              | 231±0.6μs           |    0.52 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(1, 'dense')               |
| -        | 1.44±0.03ms          | 672±7μs             |    0.47 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(2, 'dense')               |
| -        | 4.87±0.05ms          | 1.63±0.01ms         |    0.34 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(3, 'dense')               |
| -        | 368±1μs              | 206±2μs             |    0.56 | polys.TimeGCD_SparseGCDHighDegree.time_op(1, 'dense')                |
| -        | 2.37±0.04ms          | 1.20±0.01ms         |    0.5  | polys.TimeGCD_SparseGCDHighDegree.time_op(3, 'dense')                |
| -        | 9.89±0.08ms          | 4.25±0.02ms         |    0.43 | polys.TimeGCD_SparseGCDHighDegree.time_op(5, 'dense')                |
| -        | 353±2μs              | 165±1μs             |    0.47 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(1, 'dense')            |
| -        | 2.47±0.01ms          | 866±5μs             |    0.35 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(3, 'dense')            |
| -        | 9.29±0.07ms          | 2.60±0.03ms         |    0.28 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(5, 'dense')            |
| -        | 1.00±0.01ms          | 427±5μs             |    0.43 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'dense')           |
| -        | 1.70±0.01ms          | 511±2μs             |    0.3  | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse')          |
| -        | 5.68±0.01ms          | 1.77±0.01ms         |    0.31 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'dense')           |
| -        | 8.31±0.2ms           | 1.51±0ms            |    0.18 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse')          |
| -        | 283±0.9μs            | 63.6±0.6μs          |    0.22 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse')             |
| -        | 3.33±0.02ms          | 389±6μs             |    0.12 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'dense')              |
| -        | 3.83±0.06ms          | 281±0.5μs           |    0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse')             |
| -        | 6.73±0.06ms          | 1.25±0.01ms         |    0.19 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'dense')              |
| -        | 8.38±0.04ms          | 853±8μs             |    0.1  | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse')             |
| -        | 4.99±0.02ms          | 3.01±0.01ms         |    0.6  | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| -        | 11.9±0.08ms          | 6.35±0.03ms         |    0.54 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'dense')  |
| -        | 22.1±0.09ms          | 9.16±0.02ms         |    0.41 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| -        | 5.27±0.01ms          | 866±5μs             |    0.16 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse')    |
| -        | 12.6±0.05ms          | 7.10±0.02ms         |    0.56 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse')    |
| -        | 99.7±0.5ms           | 25.4±0.08ms         |    0.25 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'dense')     |
| -        | 165±0.6ms            | 54.5±0.4ms          |    0.33 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse')    |
| -        | 173±0.9μs            | 113±2μs             |    0.65 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'dense')      |
| -        | 355±1μs              | 215±2μs             |    0.61 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'sparse')     |
| -        | 4.13±0.04ms          | 842±4μs             |    0.2  | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'dense')      |
| -        | 5.10±0.02ms          | 383±2μs             |    0.07 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse')     |
| -        | 19.0±0.05ms          | 2.76±0.01ms         |    0.15 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'dense')      |
| -        | 22.1±0.1ms           | 623±3μs             |    0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse')     |
| -        | 478±2μs              | 133±0.4μs           |    0.28 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| -        | 4.57±0.03ms          | 609±2μs             |    0.13 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'dense')  |
| -        | 5.19±0.03ms          | 137±2μs             |    0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| -        | 12.8±0.06ms          | 1.27±0ms            |    0.1  | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'dense')  |
| -        | 13.7±0.07ms          | 139±1μs             |    0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| -        | 134±0.5μs            | 72.7±1μs            |    0.54 | solve.TimeMatrixOperations.time_rref(3, 0)                           |
| -        | 251±0.6μs            | 86.8±0.2μs          |    0.35 | solve.TimeMatrixOperations.time_rref(4, 0)                           |
| -        | 23.9±0.06ms          | 10.1±0.03ms         |    0.42 | solve.TimeSolveLinSys189x49.time_solve_lin_sys                       |

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

@arnabnandikgp
Copy link
Contributor Author

@anutosh491 I verified your suggested changes and also added all combinations of tests for I and oo for both atan and acot.

Copy link
Member

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working on this. Was a long time pending issue !

Could you please squash all the commits . 4 commits look unnecessary here. Once that is done I'll merge the PR @arnabnandikgp

Signed-off-by: arnabnandikgp <arnabnandi2002@gmail.com>
@anutosh491 anutosh491 merged commit 6c66027 into sympy:master Nov 28, 2023
60 checks passed
@anutosh491
Copy link
Member

anutosh491 commented Nov 28, 2023

Hey @arnabnandikgp I see that you have been addressing issues related to the series module. Here's another interesting issue which we should tackle #24266 :)
Let's try and fix this !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asymptotic series for atan/acot functions shifted to their branch cuts gives wrong answer
4 participants