-
Notifications
You must be signed in to change notification settings - Fork 5
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
Support float/double types #26
base: master
Are you sure you want to change the base?
Conversation
…allel tests for floats; added tests for invalid float operations;
super(Cint, self).__init__(val) | ||
elif isinstance(val, ctypes._SimpleCData): | ||
super(Cint, self).__init__(val.value) | ||
elif isinstance(val, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... since this is a Cint
class maybe we should have a Cfloat
class and put this (and other) logic there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we could just support str
as super(T, self).__init__(val)
since:
In [5]: float('-0.0')
Out[5]: -0.0
In [6]: float('NAN')
Out[6]: nan
In [7]: float('NAn')
Out[7]: nan
In [8]: float('nAn')
Out[8]: nan
In [9]: float('-nAn')
Out[9]: nan
In [10]: float('-InF')
Out[10]: -inf
and int('123')
works as well
return cls | ||
elif not isinstance(other, INTS): | ||
elif not isinstance(other, (INTS, FLOATS)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised that this works as it is kinda undocumented case:
In [1]: isinstance(1, (int, float))
Out[1]: True
In [2]: isinstance(1, ((int, bool), (float, str)))
Out[2]: True
In [3]: isinstance?
Signature: isinstance(obj, class_or_tuple, /)
Docstring:
Return whether an object is an instance of a class or of a subclass thereof.
A tuple, as in ``isinstance(x, (A, B, ...))``, may be given as the target to
check against. This is equivalent to ``isinstance(x, A) or isinstance(x, B)
or ...`` etc.
Type: builtin_function_or_method
In [4]: isinstance(None, ((int, bool), (float, str)))
Out[4]: False
return self.__stronger_type(other)(calc(other) // self.value) | ||
|
||
# __rdiv__ is Python 2 only | ||
__rdiv__ = __rfloordiv__ = __rtruediv__ | ||
|
||
def __mod__(self, other): | ||
if isinstance(other, (float, F32, F64)) or isinstance(self.value, (float, F32, F64)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had a CFloat
type, then we could just keep this unimplemented by not defining the method at all?
raise ValueError("Cannot perform arithmetic operations between %s and %s" % (cls, type(other))) | ||
|
||
other = other.__class__ | ||
|
||
# FLOATS are *stronger* than INTS | ||
if (cls in INTS and other in FLOATS) or (cls in FLOATS and other in INTS): | ||
return other if cls in INTS else cls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the upper scope if is not so readable, maybe it should be if ... elif
, I'll think about it more later.
# MIN/MAX are not defined for floats | ||
# Update: well, maybe they are definded after all | ||
MIN = 2 ** -126 | ||
MAX = (2 - 2 ** -23) * 2 ** 127 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth linking some resource how we computed this ;D
SIZE = 4 | ||
|
||
@classmethod | ||
def from_le(cls, val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to have such methods for any type. Also, this could be defined in a base class and just use self.SIZE
as size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work overall but see my comments for minor changes :)
No description provided.