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

[GSoC] Functions: Fixes periodicity of trigonometric function #19741

Merged
merged 1 commit into from Jul 13, 2020

Conversation

sachin-4099
Copy link
Contributor

@sachin-4099 sachin-4099 commented Jul 10, 2020

Fixes: #17976
Closes: #18277

Brief description of what is fixed or changed

Previously:

In [1]: n = Symbol('n', integer=True)
In [2]: sin(2*n*pi + 4)
Out[2]: sin(2*n*pi + 4)

Now:

In [1]: n = Symbol('n', integer=True)
In [2]: sin(2*n*pi + 4)
Out[2]: sin(4)

Other Comments

Regression Tests have been added.

Release Notes

  • functions
    • reduced symbolic multiples of pi in trigonometric functions

@sympy-bot
Copy link

sympy-bot commented Jul 10, 2020

Hi, I am the SymPy bot (v160). 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

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

Fixes: #17976
Closes: #18277


#### Brief description of what is fixed or changed

**Previously:**
```
In [1]: n = Symbol('n', integer=True)
In [2]: sin(2*n*pi + 4)
Out[2]: sin(2*n*pi + 4)
```
**Now:**
```
In [1]: n = Symbol('n', integer=True)
In [2]: sin(2*n*pi + 4)
Out[2]: sin(4)
```


#### Other Comments
Regression Tests have been added.


#### Release Notes


<!-- BEGIN RELEASE NOTES -->
* functions
  * reduced symbolic multiples of pi in trigonometric functions
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #19741 into master will decrease coverage by 0.033%.
The diff coverage is 86.666%.

@@              Coverage Diff              @@
##            master    #19741       +/-   ##
=============================================
- Coverage   75.709%   75.675%   -0.034%     
=============================================
  Files          659       662        +3     
  Lines       171401    172168      +767     
  Branches     40437     40605      +168     
=============================================
+ Hits        129766    130289      +523     
- Misses       35982     36181      +199     
- Partials      5653      5698       +45     

@sachin-4099 sachin-4099 requested a review from oscarbenjamin Jul 11, 2020
@sachin-4099 sachin-4099 requested review from oscarbenjamin and smichr and removed request for smichr Jul 11, 2020
@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 12, 2020

Stronger checks have been added to ensure better and beneficial results:

    if final_coeff.is_rational:
        if not (sin(m2).has(sin) and cos(m2).has(cos)):
            return Add(*(rest_terms + [m1])), m2

Here if final_coeff.is_rational: ensures that we are checking if we get an unevaluated result from mod.
Thus making sure that we don't encounter any RecursionError.

Here if not (sin(m2).has(sin) and cos(m2).has(cos)): has been added to make sure that returning m2 is beneficial or not.

Thus, with this changed behaviour we have:

In [1]: k, m = symbols('k m', integer = True)

In [2]: cos(k*pi + (m - 1)*pi/2 + 1)
Out[2]:
   /       pi*(m - 1)    \
cos|pi*k + ---------- + 1|
   \           2         /

In [3]: cos(1+m*pi/2)
Out[3]:
   /pi*m    \
cos|---- + 1|
   \ 2      /

@sachin-4099 sachin-4099 requested a review from oscarbenjamin Jul 12, 2020
@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 12, 2020

Instead of this:

    final_coeff = m2 / S.Pi
    if final_coeff.is_rational:
        if not (sin(m2).has(sin) and cos(m2).has(cos)):
            return Add(*(rest_terms + [m1])), m2
    return arg, S.Zero

If we have this:

    final_coeff = m2 / S.Pi
    if final_coeff.is_integer or ((2*final_coeff).is_integer and final_coeff.is_even is False):
            return Add(*(rest_terms + [m1])), m2
    return arg, S.Zero

What do you think @oscarbenjamin?

@sachin-4099 sachin-4099 requested a review from oscarbenjamin Jul 13, 2020
@oscarbenjamin oscarbenjamin changed the title [GSoC] Functions: Fixes periodicty of trigonometric function [GSoC] Functions: Fixes periodicity of trigonometric function Jul 13, 2020
@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jul 13, 2020

I think this looks okay. Can you squash the commits and give a new commit with a better message?

Maybe something like:

feat(functions): reduce symbolic multiples of pi in trig functions

It's worth thinking when you write the commit messages that they end up being looked at outside of the context of the pull-request in which the commits are merged. Their purpose is not to be seen here but rather to be seen in the git log or git blame. Many commit messages are not meaningful in those contexts:
https://github.com/sympy/sympy/commits/master
https://github.com/sympy/sympy/blame/9dec25dcd8139fb1b35d7d6b49e22da120262afd/sympy/functions/elementary/trigonometric.py#L113

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 13, 2020

It's worth thinking when you write the commit messages that they end up being looked at outside of the context of the pull-request in which the commits are merged. Their purpose is not to be seen here but rather to be seen in the git log or git blame. Many commit messages are not meaningful in those contexts:

In my opinion, one should reach out to the PR [from the commit hash] rather than trying to understand the purpose of the changes from the commit messages.

One can not come up with meaningful commit messages for every commit, and thus most of them will seem to be outside of the context of the pull-request, until one reaches out to the PR and then the changes brought by each commit will try to make sense along with the commit messages.

@sachin-4099 sachin-4099 requested a review from oscarbenjamin Jul 13, 2020
@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jul 13, 2020

In my opinion, one should reach out to the PR [from the commit hash] rather than trying to understand the purpose of the changes from the commit messages.

I strongly disagree. The commit messages are accessible in may ways without going through github. I sent links to the github interface for log and blame but I use those as commands in the terminal where there is no concept of a pull request. The commit messages might outlive github and still be there when sympy has moved to a new hosting platform at which point the pull request might not exist any more.

This already happened once when sympy moved from google code to github. As a result many old issue numbers referred to in the code are incorrect. The authors at the time of writing did not consider that a different hosting platform might exist so they referred to "issue 122" without any context for the number. That's why I prefer to refer to an issue in the code with a URL like https://github.com/sympy/sympy/pull/19741 because it is then unambiguous that the issue number refers to github rather than some future platform.

You are correct that right now the most useful way to find out the changes from a commit is often to navigate through to the pull request but that is at least partly because we have so many poor quality commit messages in master.

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 13, 2020

The commit messages might outlive github and still be there when sympy has moved to a new hosting platform at which point the pull request might not exist any more.

This makes sense.

Btw, the PR looks good and can be merged @oscarbenjamin.

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jul 13, 2020

Im very wary of automatic evaluation especially in core functions like this but this looks reasonable.

Thanks

@oscarbenjamin oscarbenjamin merged commit 586a432 into sympy:master Jul 13, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Periodicity of trigonometric function
3 participants