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

Add Half-Precision Type #509

Merged
merged 5 commits into from
Sep 30, 2019
Merged

Add Half-Precision Type #509

merged 5 commits into from
Sep 30, 2019

Conversation

njwhite
Copy link
Contributor

@njwhite njwhite commented Sep 11, 2019

In the style of Float and Double

@seibert seibert requested a review from sklam September 11, 2019 21:16
@seibert
Copy link
Contributor

seibert commented Sep 11, 2019

related: numba/numba#4402

@njwhite
Copy link
Contributor Author

njwhite commented Sep 12, 2019

CI failures seem spurious?

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch, couple of comments to resolve then this is good to go.

return struct.unpack('e', struct.pack('e', value))[0]
except struct.error:
# 'e' only added in Python 3.6+
return _as_float(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might this be a source of confusion, a user asks for a half but when formatted it appears to not have been correctly truncated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could use numpy to round it (the logic is complicated!), but I don't think llvmlite depends on that at the minute. Is that OK? I'd rather use a library to do the rounding - especially as python 2.x is decreasing in popularity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sklam do you have a view on this. Perhaps just documenting that it's different or issuing a warning is the easiest thing to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not rounded properly, at parse_assembly(), it will raise RuntimeError with:

RuntimeError: LLVM IR parsing error
<string>:5:27: error: floating point constant invalid for type
@"myhalf" = constant half 0x3f87006680000000

I think we should just document it. Telling user to consider doing something like float(np.float16(1.12312321e-2)) to make sure the value can be exactly represented as half.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sklam @stuartarchibald I've pushed a change to the documentation that covers this

from . import TestCase


int8 = IntType(8)
int16 = IntType(16)


PY36 = sys.version_info[:2] >= (3, 6)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PY36 = sys.version_info[:2] >= (3, 6)
PY36_OR_LATER = sys.version_info[:2] >= (3, 6)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed this

@stuartarchibald
Copy link
Contributor

Close/Open to restart all of CI easily. Apologies for the noise.

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes.

@sklam sklam added this to the Version 0.30.0 milestone Sep 30, 2019
@sklam sklam merged commit 5f51d4d into numba:master Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants