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

Added math module #33

Merged
merged 9 commits into from Jan 9, 2017

Conversation

Projects
None yet
8 participants
@lalaithion
Copy link
Contributor

lalaithion commented Jan 5, 2017

This request contains a shim package that wraps Go functions from the math package in functions that are identical to those in the Python math module. There are three comments that begin with "NOTE", that I think require input from others and probably some corrections.

lalaithion added some commits Jan 5, 2017

lib/math.py Outdated
def factorial_helper(x, acc):
if x <= 1:
return acc
else:

This comment has been minimized.

@matbur

matbur Jan 5, 2017

Is this else statement necessary?

lib/math.py Outdated

# Number-theoretic and representation functions

def ceil(x):

This comment has been minimized.

@maciej-gol

maciej-gol Jan 5, 2017

Why not an import with alias, like

from __go__.math import (
    Pi as pi,
    E as e,
    Ceil as ceil,
    ...
)

?

This comment has been minimized.

@denismakogon
@trotterdylan
Copy link
Collaborator

trotterdylan left a comment

Thanks for the PR! Overall, this looks great, just a few minor issues to resolve.

lib/math.py Outdated

if x % 1 != 0 or x < 1:
raise ValueError
else:

This comment has been minimized.

@trotterdylan

trotterdylan Jan 5, 2017

Collaborator

Another unnecessary else

This comment has been minimized.

@denismakogon
lib/math.py Outdated
def ceil(x):
return Ceil(x)

def copysign(x,y):

This comment has been minimized.

@trotterdylan

trotterdylan Jan 5, 2017

Collaborator

Please put two blank lines between top level definitions: https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines

This comment has been minimized.

@denismakogon
lib/math.py Outdated
def copysign(x,y):
return Copysign(x,y)

def fabs(x):

This comment has been minimized.

@trotterdylan

trotterdylan Jan 5, 2017

Collaborator

Inputs should be converted to floats so that the following works:

>>> class A(object):
...   def __float__(self):
...     return 3.14
... 
>>> math.fabs(A())
3.14

Applies to most functions in this file.

lib/math.py Outdated
def ceil(x):
return Ceil(x)

def copysign(x,y):

This comment has been minimized.

@denismakogon
lib/math.py Outdated

if x % 1 != 0 or x < 1:
raise ValueError
else:

This comment has been minimized.

@denismakogon
lib/math.py Outdated
if b is None:
return Log(x)
else:
# NOTE: We can try and catch more special cases to delegate to specific

This comment has been minimized.

@tusharmakkar08

tusharmakkar08 Jan 5, 2017

Another unnecessary else.

Arguments now wrapped in call to float
other changes:
unnecessary elses removed
double newline after top level definitions added
@lalaithion

This comment has been minimized.

Copy link
Contributor Author

lalaithion commented Jan 5, 2017

I just pushed most of the changes suggested by this thread.

lib/math.py Outdated


def copysign(x,y):
return Copysign(float(x),float(y))

This comment has been minimized.

@trotterdylan

trotterdylan Jan 6, 2017

Collaborator

Spaces after commas in function arguments.

lib/math.py Outdated
return Frexp(float(x))


# NOTE: This function exists in python, but I don't know how to write it,

This comment has been minimized.

@trotterdylan

trotterdylan Jan 6, 2017

Collaborator

Maybe just make this a TODO: implement fsum

lib/math.py Outdated


def modf(x):
#Modf returns (int, frac), but python should return (frac, int)

This comment has been minimized.

@trotterdylan

trotterdylan Jan 6, 2017

Collaborator

Space after # in comments. Also please make comments full sentences ending with period.

lib/math.py Outdated
def modf(x):
#Modf returns (int, frac), but python should return (frac, int)
(a, b) = Modf(float(x))
return (b, a)

This comment has been minimized.

@trotterdylan

trotterdylan Jan 6, 2017

Collaborator

Remove extraneous parentheses here and on previous line

lib/math.py Outdated

# NOTE: We can try and catch more special cases to delegate to specific
# Go functions or maybe there is a function that does this and I missed it
return Log(float(x))/Log(float(b))

This comment has been minimized.

@trotterdylan

trotterdylan Jan 6, 2017

Collaborator

Spaces around operators: Log(float(x)) / Log(float(b))

lib/math.py Outdated


def pow(x, y):
return Pow(float(x),float(y))

This comment has been minimized.

@trotterdylan

trotterdylan Jan 6, 2017

Collaborator

Space after ,

lib/math.py Outdated
if x % 1 != 0 or x < 1:
raise ValueError

return factorial_helper(int(x),1)

This comment has been minimized.

@ns-cweber

ns-cweber Jan 6, 2017

Contributor

According to the docs, this should raise ValueError if x is not integral. Also, I'm not sure how much the exception messages matter, but here are the exact messages raised by python2.7:

>>> math.factorial(2.1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: factorial() only accepts integral values
>>> math.factorial(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: factorial() not defined for negative values
lib/math.py Outdated

def factorial(x):

def factorial_helper(x, acc):
if x <= 1:
return acc
else:
return factorial_helper(x - 1, acc * x)
return factorial_helper(x - 1, acc * x)

if x % 1 != 0 or x < 1:

This comment has been minimized.

@matbur

matbur Jan 6, 2017

Expected behavior for factorial function is:

>>> factorial(0)
1

It raises ValueError instead.

factorial has better error messages and accepts 0
other style fixes changed as well
@ns-cweber

This comment has been minimized.

Copy link
Contributor

ns-cweber commented Jan 7, 2017

The code looks good to me, but maybe factorial deserves a test(s)?

@lalaithion

This comment has been minimized.

Copy link
Contributor Author

lalaithion commented Jan 8, 2017

I can't figure out how to write an implementation of factorial that preserves this behavior, without explicit checks for certain types:

Python 2.7.10 (v2.7.10:15c95b7d81dc, May 23 2015, 09:33:12) 
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A(object):
...     def __int__(self):
...             return 2
... 
>>> class B(object):
...     def __float__(self):
...             return 2.2
... 
>>> from math import factorial
>>> factorial(2)
2
>>> factorial(2.1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: factorial() only accepts integral values
>>> a = A()
>>> b = B()
>>> factorial(a)
2
>>> factorial(b)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: an integer is required

I'm not sure how compliant to this sort of behavior we want. We can't just convert things to integers, as that would allow any floating point value.

@ns-cweber

This comment has been minimized.

Copy link
Contributor

ns-cweber commented Jan 8, 2017

Wow, that is strange. What do CPython and PyPy do?

@ns-cweber

This comment has been minimized.

Copy link
Contributor

ns-cweber commented Jan 8, 2017

Here's the source for PyPy's factorial(): https://bitbucket.org/pypy/pypy/src/f3abf62ba36410343903047f980846f023bb3322/pypy/module/math/app_math.py?at=default&fileviewer=file-view-default. In particular, it looks like factorial(2.0) should succeed. According to #60, it might be fine just to pull this source file in directly rather than reinventing the wheel.

@S-YOU

This comment has been minimized.

Copy link
Contributor

S-YOU commented Jan 8, 2017

While pypy's approach is definitely performant, but it is using side effect of comparing class with number, and error is ValueError: x must be >= 0, instead of TypeError: an integer is required for his example factorial(b)

if x < 0:
            raise ValueError("x must be >= 0")

May be use type checking for int, float or raise TypeError?

@ns-cweber

This comment has been minimized.

Copy link
Contributor

ns-cweber commented Jan 8, 2017

@S-YOU I agree that PyPy uses a ValueError instead of a TypeError for the non-integral argument exception, but I think its error is correct in the negative argument case. I think you might be conflating the cases of non-integral argument (e.g., 2.2) and negative argument (e.g., -2). I don't know how important it is that we return a TypeError instead of a ValueError in the case of non-integral arguments.

@lalaithion If you don't want to use the PyPy function entirely, I think you could solve your immediate problem by doing:

if int(x) != x:
    raise TypeError
Made factorial() more performant
It also makes better checks for the input.
Part of factorial() taken from PyPy's version.
@lalaithion

This comment has been minimized.

Copy link
Contributor Author

lalaithion commented Jan 8, 2017

I decided to go for a more accepting check on types than either PyPy or CPython seem to have.

If you have a class that has an int function, it calls that and uses that.
If you have a class that has an float function, it calls that and makes sure the float is integral.
If you have a class that has both, it calls them both and makes sure they are the same.

This seems to allow for all the behavior we might want while preventing any non-integral numbers from being evaluated.

I also copied the more performant version of calculating the factorial from PyPy.

@trotterdylan
Copy link
Collaborator

trotterdylan left a comment

Unfortunately we'd be violating the terms of the PyPy license to copy the factorial implementation into this file. If we're going to copy it, we need to put it into a third_party/ subdir with an appropriate license.

I'm fine with the unoptimized implementation for now. We can always optimize as needed.

@@ -0,0 +1,244 @@
from __go__.math import (Pi, E, Ceil, Copysign, Abs, Floor, Mod, Frexp, IsInf,

This comment has been minimized.

@trotterdylan

trotterdylan Jan 9, 2017

Collaborator

Please add the file header here and in math_test.py:

# Copyright 2016 Google Inc. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
Removed PyPy factorial() implementation
Added liscenses to top of files
@lalaithion

This comment has been minimized.

Copy link
Contributor Author

lalaithion commented Jan 9, 2017

If we want a faster factorial() function, it should probably be implemented in Go anyway.

@trotterdylan

This comment has been minimized.

Copy link
Collaborator

trotterdylan commented Jan 9, 2017

This is great. Thanks for pushing this through!

@trotterdylan trotterdylan merged commit dfc59c2 into google:master Jan 9, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment