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

Float.is_integer updated for removing disparity in gamma #16897

Merged
merged 5 commits into from May 28, 2019

Conversation

Projects
None yet
5 participants
@czgdp1807
Copy link
Member

commented May 26, 2019

References to other Issues or PRs

[1] Fixes #16847
[2] #16847 (comment)

Brief description of what is fixed or changed

Float.is_integer now returns True for values like, -1.0, which helps in removing disparity as mentioned in the issue referenced above.

Other comments

ping @smichr @jksuom @ritesh99rakesh

The following test is failing due to the changes,

__________ sympy/core/tests/test_power.py:test_issue_6100_12942_4473 ___________
Traceback (most recent call last):
  File "/home/gagandeep/sympy/sympy/core/tests/test_power.py", line 284, in test_issue_6100_12942_4473
    assert ((x*y)**1.0).func is Pow
AssertionError

(x*y)**1.0 is converted to x**1.0*y**1.0 returning Mul instead of Pow therefore failing the test.

Release Notes

NO ENTRY

@sympy-bot

This comment has been minimized.

Copy link

commented May 26, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write 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.

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
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
[1] Fixes https://github.com/sympy/sympy/issues/16847
[2] https://github.com/sympy/sympy/issues/16847#issuecomment-493653572

#### Brief description of what is fixed or changed
Float.is_integer now returns `True` for values like, `-1.0`,  which helps in removing disparity as mentioned in the issue referenced above. 

#### Other comments
ping @smichr @jksuom @ritesh99rakesh 

The following test is failing due to the changes,
```
__________ sympy/core/tests/test_power.py:test_issue_6100_12942_4473 ___________
Traceback (most recent call last):
  File "/home/gagandeep/sympy/sympy/core/tests/test_power.py", line 284, in test_issue_6100_12942_4473
    assert ((x*y)**1.0).func is Pow
AssertionError
```
`(x*y)**1.0` is converted to `x**1.0*y**1.0` returning `Mul` instead of `Pow` therefore failing the test.

#### Release Notes

<!-- Write the release notes for this release below. 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 -->
NO ENTRY
<!-- END RELEASE NOTES -->

Show resolved Hide resolved sympy/core/numbers.py Outdated
@smichr

This comment has been minimized.

Copy link
Member

commented May 27, 2019

I don't think you want to change Float(1).is_integer to be True. The lower-case variety means that you have a symbolic integer (as opposed to a literal Integer).

Floats are a special subset of Rationals. Although there is no inherent uncertainty in the representation of a given Float, each float must represent numbers which are too close to it to be represented with finite precision. So even Float(1) -- though representationally equal to 1 -- stands for 1 +/- eps and should not be used to separate powers as in (x*y)**1.0 to x**1.0*y**1.0.

But what about gamma(Float(-1))? Even though we might say that Float(-1) does not exactly stand for -1, it is -1: all numbers in [-1-eps, -1+eps] are being represented as -1 (like numbers underflowing to 0) and we can't compute gamma at -1.

So either Float(1).is_integer becomes True and all tests for foo.is_integer become foo.is_integer and not foo.is_Float -- that's not a good solution. Or, for places where you are looking for a Number that is representationally the same as an Integer, use as_int(foo, strict=False):

>>> if as_int(Float(1), strict=False): print('ok')
...
ok
Show resolved Hide resolved sympy/core/numbers.py Outdated
@smichr

This comment has been minimized.

Copy link
Member

commented May 27, 2019

p.s. sorry about the typo that suggested this PR would close because of the one I was working on!

@jksuom

This comment has been minimized.

Copy link
Member

commented May 27, 2019

all tests for foo.is_integer become foo.is_integer and not foo.is_Float

I'm not sure I understand why this would be necessary. We have .is_Integer for those cases where an Integer object is needed. But otherwise integer valued floats will compare equal to proper integers so it looks like they should also be accepted as 'integers'. (But maybe I don't see the whole picture.)

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

But otherwise integer valued floats will compare equal to proper integers so it looks like they should also be accepted as 'integers'. (But maybe I don't see the whole picture.)

I agree with @smichr here. Float(1.0) and 1 are different as in 1.0 precision matters but 1 is a constant with infinite precision. AFAIK, there is always an error of the max precision. If I am wrong then please correct me, at least my concepts about floats and integers in general would be corrected. For now, I will make the change as @smichr suggested. Thanks.

@czgdp1807 czgdp1807 changed the title [GSoC] Float.is_integer updated for removing disparity in gamma Float.is_integer updated for removing disparity in gamma May 27, 2019

@codecov

This comment has been minimized.

Copy link

commented May 27, 2019

Codecov Report

Merging #16897 into master will decrease coverage by 0.002%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #16897       +/-   ##
=============================================
- Coverage   73.906%   73.903%   -0.003%     
=============================================
  Files          619       619               
  Lines       160001    160029       +28     
  Branches     37554     37558        +4     
=============================================
+ Hits        118251    118267       +16     
- Misses       36269     36286       +17     
+ Partials      5481      5476        -5
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 27, 2019

@smichr Tests passing. Is it good to merge?

@smichr

This comment has been minimized.

Copy link
Member

commented May 27, 2019

looks like they should also be accepted as 'integers'

This is like the face/vase optical illusion:
image

A Float is a subset of Rationals and is exact. When used in a calculation it will give an approximate result. Even though all numbers [f-eps, f+eps] are mapped to f, that is our problem, not f's problem. So when trying to decide if (x*y)**1.0 should be expanded as x**1.0*y**1.0 we must choose to do so. Refusingto do so because there are other numbers that could be 1.0 is the "face"; to do so because the 1.0 that we have is identically 1 is the "vase".

To me it seem like the result that we calculate could be exact, but currently that is not how SymPy behaves:

>>> pi.n()**Float(1,2) == pi.n()**Float(1)
True

Whether that should be False is not lilkely to change since

>>> mlib.mpf_mul(Float(.1)._mpf_,Float(1,2)._mpf_)
(0, 3602879701896397, -55, 52)
>>> mlib.mpf_mul(Float(.1)._mpf_,Float(1,22)._mpf_)
(0, 3602879701896397, -55, 52)

So why not replace input to Float that has a power-of-2 denominator with a Rational? That's another issue. Presumably this would not be a trouble with code generation because each language printer would have to figure out if 1/2 should be printed like that or as 1/2.0 given the context.

I am in favor of the previous modifications and an update to the test to show (x*y)**1.0 -> x**1.0*y**1.0.

self-updating test for intlike redundancy
When the test fails it directs the author to edit gamma_functions to remove the redundant intlike routine.
@smichr

This comment has been minimized.

Copy link
Member

commented May 27, 2019

The details of handling Floats as ints/rationals may take a while to work out and this patch is a reasonable interim solution. I added a test that directs the author to remove intlike if Float(1).is_integer ever becomes True.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

I don't see what's gained by preventing (x*y)**1.0 -> x**1.0*y**1.0 but I agree that this PR shouldn't be held up over that.

We really need to get a clear vision for what to do with floats but it's difficult to get there just by sporadic discussion on myriad unrelated issues

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

@oscarbenjamin @smichr I opened this PR for correcting the disparity in gamma function and not floats. As far as I can figure out that is solved by @smichr 's suggestion and that too without affecting the Float which I think is quite acceptable.
For discussing over Float we can have an issue and a separate PR fixing that.
I will think upon the recent comments made after the last commit and will write my view in the comments.

@smichr smichr merged commit 25d8302 into sympy:master May 28, 2019

3 checks passed

codecov/project 73.903% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details

@czgdp1807 czgdp1807 deleted the czgdp1807:gamma_int branch May 28, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

Thanks @smichr. I will shortly raise an issue for discussing over Float, especially the is_integer attribute.
However, if there is any such pre-existing method, then please ping me there. I want to be a part of that discussion. Thanks. :)

@jksuom

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Changing Float.is_integer is probably not good but it would be possible to use the exponent criterion directly: arg.is_Float and arg.num.exp >= 0.

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.