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

Refinements in discrete module #15025

Merged
merged 3 commits into from Aug 6, 2018

Conversation

Projects
None yet
4 participants
@sidhantnagpal
Copy link
Member

sidhantnagpal commented Aug 4, 2018

Brief description of what is fixed or changed

  • Fix the floats in test_convolutions.py which should be rationals (Thanks @cbm755)
  • Add warning about sequence size for fft
  • Remove very basic doctests in fft
  • Use is False instead of == False checks

Release Notes

NO ENTRY

cbm755 and others added some commits Jul 26, 2018

convolution: fix many accidental floats in tests
I don't think these are supposed to be python floats; many other
nearby quantities use `S` to ensure they are not floats.  So do more
of that.
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Aug 4, 2018

Hi, I am the SymPy bot (v124). I'm here to make sure this pull request has a release notes entry. Please read the guide on how to write release notes.

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

#### Brief description of what is fixed or changed
- Fix the floats in `test_convolutions.py` which should be rationals (Thanks @cbm755)
- Add warning about sequence size for `fft` 
- Remove very basic doctests in `fft`
- Use `is False` instead of `== False` checks

#### Release Notes
<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

No release notes entry will be added for this pull request.

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.

@@ -141,7 +139,7 @@ def _number_theoretic_transform(seq, prime, inverse=False):
"for Number Theoretic Transform")

p = as_int(prime)
if isprime(p) == False:
if isprime(p) is False:

This comment has been minimized.

@jksuom

jksuom Aug 4, 2018

Member

Maybe not isprime(p)?

This comment has been minimized.

@sidhantnagpal

sidhantnagpal Aug 4, 2018

Author Member

Yes, that is possible. Could isprime(p) return None?

This comment has been minimized.

@jksuom

jksuom Aug 4, 2018

Member

I don't know for sure but I doubt it.

@@ -876,10 +876,10 @@ def eval(cls, n, k):
n, k = map(sympify, (n, k))
d = n - k
n_nonneg, n_isint = n.is_nonnegative, n.is_integer
if k.is_zero or ((n_nonneg or n_isint == False)
if k.is_zero or ((n_nonneg or n_isint is False)

This comment has been minimized.

@jksuom

jksuom Aug 4, 2018

Member

I am not sure of what n.is_integer or n.is_nonnegative will return in general. This is from docs:

if there is any doubt over whether a function or expression will return S.true or True, just use == instead of is to do the comparison

This comment has been minimized.

@sidhantnagpal

sidhantnagpal Aug 4, 2018

Author Member

It also mentions:

In other words, use S.true only on those contexts where the boolean is being used as a symbolic representation of truth.

I wonder if it is applicable to is_integer and is_nonnegative.

This comment has been minimized.

@sidhantnagpal

sidhantnagpal Aug 4, 2018

Author Member

I think they will be having three-valued logic:

The assumptions system should use True and False. Aside from not satisfying the above rule of thumb, the assumptions system uses a three-valued logic (True, False, None), whereas S.true and S.false represent a two-valued logic. When in doubt, use True.

@Abdullahjavednesar Abdullahjavednesar requested a review from jksuom Aug 5, 2018

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Aug 6, 2018

Thanks, looks good.

@jksuom jksuom merged commit bc1d1d9 into sympy:master Aug 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.