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 .view() method for NumPy scalars #3928

Merged
merged 31 commits into from
Apr 12, 2019
Merged

Conversation

esc
Copy link
Member

@esc esc commented Apr 2, 2019

This implements and fixes #3763

@sklam
Copy link
Member

sklam commented Apr 2, 2019

We will need to differentiate between numpy scalar and regular scalars that do not have the .view. Like:

x = 1
x.view(...) # invalid
np.int32(x).view(...) # valid

@sklam
Copy link
Member

sklam commented Apr 2, 2019

I wonder what is the status of .view on numpy scalar types. help(np.int32.view) shows:

view(...) method of numpy.int32 instance
    Not implemented (virtual attribute)

@esc
Copy link
Member Author

esc commented Apr 2, 2019

@sklam should we implement view for regular scalars too?

@sklam
Copy link
Member

sklam commented Apr 2, 2019

@esc, the problem is that numba treat numpy scalar type and regular scalar type in the same way. So you have already implemented .view for regular scalar. But that will not work for regular python.

@esc
Copy link
Member Author

esc commented Apr 3, 2019

Travis failed, because job exceeded timeout, restarting and keeping fingers crossed.

@esc
Copy link
Member Author

esc commented Apr 3, 2019

@sklam is there anything else I might be able to code up for this? I don't think we should add a view method to regular Python types.

@esc
Copy link
Member Author

esc commented Apr 3, 2019

Seems like it exceeded the time-limit again. Asking Travis-CI people if they can increase the timeout for Numba builds for us.

@seibert
Copy link
Contributor

seibert commented Apr 3, 2019

the Travis CI builds are redundant to the Azure builds at this point. Should we just drop them?

@esc
Copy link
Member Author

esc commented Apr 4, 2019

@seibert it may really be worth considering.

@esc
Copy link
Member Author

esc commented Apr 5, 2019

Looks like this is failing for Python 2.7

@esc
Copy link
Member Author

esc commented Apr 5, 2019

Perhaps some sort of ascii vs. unicode issue, certainly does smell like it.

@stuartarchibald
Copy link
Contributor

Unicode (unicode_type) is not supported on Py27 which is the root cause of the problems in CI, am wondering if a better error message could be raised (not for this PR though).

@esc
Copy link
Member Author

esc commented Apr 5, 2019

So I updated the tests and they pass locally on 2.7. I hope they will pass CI too. @stuartarchibald @sklam is there any other types where you suggest I test them? int8 -> uint8 perhaps? Also, probably worth documenting that only Numpy scalars are supported and that they need to be initialized within the jitted function. Thoughts on where to drop this?

Two side quests so far:

  • consider turning off TravisCI
  • better errors on 2.7 for unsupported unicode stuffs.

@esc
Copy link
Member Author

esc commented Apr 5, 2019

@stuartarchibald, actually, I think that raise statement can never be reached. Using overload_method means that the view is only available for integer and floating point types. Any other type will lead to something like: Unknown attribute 'view' of type complex128 with complex128 being the type. Do you agree? If so, it probably makes sense to remove it.

@stuartarchibald
Copy link
Contributor

As the intrinsic is a function it could well be reused, but is only likely to be by developers, given that the implementation returned could be invalid in the case of simply removing the exception I'm inclined to leave it in or replace it with assert 0, 'unreachable'. The intrinsic function could also be moved to numba.unsafe.number or something as it is a useful piece of functionality.

@svenrahmann
Copy link

This implements exactly what I was looking for. Thank you.

@esc
Copy link
Member Author

esc commented Apr 6, 2019

@svenrahmann thanks for the feedback, happy to hear it! 🎉

@esc
Copy link
Member Author

esc commented Apr 6, 2019

CI revealed some tests that should be skipped on non-64 bit systems.

@esc
Copy link
Member Author

esc commented Apr 7, 2019

All green and ready for review.

@esc esc added the 4 - Waiting on reviewer Waiting for reviewer to respond to author label Apr 7, 2019
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 PR. This in general seems good and does what is expected. I've made a few comments, once resolved I'll re-review with view to merge. Thanks again!

docs/source/reference/numpysupported.rst Outdated Show resolved Hide resolved
docs/source/reference/numpysupported.rst Outdated Show resolved Hide resolved
docs/source/reference/numpysupported.rst Outdated Show resolved Hide resolved
numba/ir_utils.py Show resolved Hide resolved
numba/ir_utils.py Outdated Show resolved Hide resolved
numba/tests/test_numbers.py Show resolved Hide resolved
numba/tests/test_numbers.py Outdated Show resolved Hide resolved
numba/tests/test_numbers.py Outdated Show resolved Hide resolved
numba/tests/test_numbers.py Show resolved Hide resolved
docs/source/reference/numpysupported.rst Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Apr 8, 2019
@stuartarchibald stuartarchibald added this to the Numba 0.44 RC milestone Apr 8, 2019
@stuartarchibald stuartarchibald changed the title bitcast scalars Add .view() method for NumPy scalars Apr 8, 2019
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, couple more things to resolve and this can be marked for merge.

docs/source/reference/numpysupported.rst Outdated Show resolved Hide resolved
numba/tests/test_numbers.py Outdated Show resolved Hide resolved
@esc
Copy link
Member Author

esc commented Apr 8, 2019

Thanks for all the feedback, pushing fixes now.

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 your persistence in getting this to work. This looks good :)

@stuartarchibald
Copy link
Contributor

Pending CI pass, this can be marked for merge.

@esc
Copy link
Member Author

esc commented Apr 8, 2019

😁

@esc
Copy link
Member Author

esc commented Apr 9, 2019

Travis had a timeout again connecting to anaconda.org restarted the build.

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Apr 9, 2019
@stuartarchibald stuartarchibald merged commit eb6b473 into numba:master Apr 12, 2019
@esc esc deleted the bitcast_scalars branch April 12, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support view method on scalars
5 participants