-
Notifications
You must be signed in to change notification settings - Fork 55
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
Feature/713 result type #761
Conversation
Codecov Report
@@ Coverage Diff @@
## master #761 +/- ##
==========================================
- Coverage 95.35% 95.35% -0.01%
==========================================
Files 64 64
Lines 8854 8891 +37
==========================================
+ Hits 8443 8478 +35
- Misses 411 413 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
looks good. minor changes and questions
heat/core/types.py
Outdated
except Exception: | ||
try: | ||
# type | ||
if isinstance(arg, np.dtype): | ||
arg = arg.char | ||
type1 = canonical_heat_type(arg) | ||
prec1 = 1 | ||
except Exception: |
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.
why such a broad exception?
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.
What do you suggest?
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.
what is raised when this happens? TypeError
?
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.
Ah - I first thought you meant the except block and not the caught exception 🤦
heat/core/types.py
Outdated
else: | ||
if prec1 == prec2: | ||
return promote_types(type1, type2), prec1 | ||
|
||
for sclass in (bool, integer, float): | ||
if issubdtype(type1, sclass) and issubdtype(type2, sclass): | ||
if prec1 < prec2: | ||
return type1, min(prec1, prec2) | ||
if prec1 > prec2: | ||
return type2, min(prec1, prec2) | ||
|
||
tc1 = __type_codes[type1] | ||
tc2 = __type_codes[type2] | ||
|
||
if tc1 < tc2: | ||
return type2, min(prec1, prec2) | ||
if tc1 > tc2: | ||
return type1, min(prec1, prec2) | ||
|
||
raise RuntimeError("Something went wrong") # pragma: no cover |
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.
can drop these down some tabs, the if loops end with returns
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.
Yes, I flattened the else block.
…ics/heat into feature/713-result-type
will merge once tests finish |
Description
This PR provides an implementation of result_type. Additionally, some changes in __binary_op on the output are introduced.
Issue/s resolved: #713
Changes proposed:
Type of change
Due Diligence
Does this change modify the behaviour of other functions? If so, which?
yes, the result type of __binary_op follows result_type and scalars are returned when adding scalars and numbers.