-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
PR: Added support to the principal numeric Numpy types in the Variable Explorer #3653
Conversation
If possible, please include this in 3.0.2. |
from numpy import ndarray, array, matrix, recarray | ||
from numpy import ndarray, array, matrix, recarray, \ | ||
int64, int32, float64, float32, complex_, \ | ||
complex64, complex128 |
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.
Please enclose imports in parenthesis
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 make these objects FakeObject
's below
elif isinstance(value, complex_) or isinstance(value, complex128) : | ||
display = repr(value) | ||
elif isinstance(value, complex64): | ||
display = repr(value) |
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.
Please create a list comprehension for this block
elif isinstance(value, complex_) or isinstance(value, complex128) : | ||
display = repr(value) | ||
elif isinstance(value, complex64): | ||
elif True in [isinstance(value, x) for x in numeric_type_list]: |
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'd write this as any(isinstance(value, x) for x in numeric_type_list)
or isinstance(value, numeric_type_tuple)
; in the latter case, I think it needs to be a tuple of types, not a list of types.
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 also like the second option. @dalthviz, please do the corresponding change :-)
@wtheis, unfortunately 3.0.2 is almost done, and we don't want to delay it longer. |
@@ -244,6 +247,8 @@ def unsorted_unique(lista): | |||
def value_to_display(value, minmax=False): | |||
"""Convert value for display purpose""" | |||
try: | |||
numeric_type_tuple = (int64, int32, float64, float32, \ |
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.
@dalthviz, I think you don't need a continuation line (\
) here
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, please rename this variable to numeric_numpy_types
@@ -290,6 +295,8 @@ def value_to_display(value, minmax=False): | |||
elif isinstance(value, NUMERIC_TYPES) or isinstance(value, bool) or \ | |||
isinstance(value, datetime.date): | |||
display = repr(value) | |||
elif isinstance(value, numeric_type_tuple): |
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.
Please move this to the elif isinstance(value, NUMERIC_TYPES) or ...
block above because it returns the same result (i.e. display = repr(value)
).
@@ -44,16 +44,19 @@ class FakeObject(object): | |||
|
|||
|
|||
#============================================================================== | |||
# Numpy arrays support | |||
# Numpy arrays and numeric support |
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.
Please change this to Numpy arrays and numeric types support
…list comprehension in the validation
08c4c7c
to
c5852d2
Compare
Fixes #3638