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

Numpy array #327

Merged
merged 4 commits into from Nov 4, 2018

Conversation

Projects
None yet
2 participants
@Thrameos
Contributor

Thrameos commented Jun 6, 2018

This pull request exposes a bug in the array slice routine in which it assumed all primitives with the same type are compatible and thus tries to use the direct copy method. This leads to rather predictable failures.

@Thrameos

This comment has been minimized.

Contributor

Thrameos commented Jun 6, 2018

This pull request is not ready to go. It does correct the basic issue that memory was pushed around inappropriately but it requires the additional step of make numpy types be recognized by the canConvertToJava. Without this everything will take the bypass and nothing will hit the optimized copy path.

@Thrameos

This comment has been minimized.

Contributor

Thrameos commented Jul 2, 2018

This one needs to be finished. Not sure yet why is does not work on *nix.

@Thrameos

This comment has been minimized.

Contributor

Thrameos commented Jul 18, 2018

Hmm. Seems that somehow I lost part of this pull request.

I did further analysis of the numpy problem. The use of a memory view without using the numpy structures to first determine how to access the data is fundamentally flawed. I started implementing a replacement under devel, but it is a pretty heavy lift because the NUMPY array api allows so many different memory organizations. My implementation will certainly be better than allowing incorrect data to be passed in so long as the size of the data types is the same, but there are a lot of edge cases. I am not sure even how I would create a misbehaving, byte order reversed, view of data with a stride. Unfortunately, that is what would be required to hit the test cases to ensure we do everything correctly.

After it is fixed in devel we can look to back port if needed.

@marscher

This comment has been minimized.

Collaborator

marscher commented Nov 4, 2018

I do not think it is possible to map non contiguous arrays to Java. Therefore we should just raise, if the array does not full fill this requirement.

@marscher marscher merged commit 7802a53 into jpype-project:master Nov 4, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@Thrameos

This comment has been minimized.

Contributor

Thrameos commented Nov 4, 2018

Seems like this is the problematic patch. Shall we revert it?

@Thrameos

This comment has been minimized.

Contributor

Thrameos commented Nov 4, 2018

The problem was a bit more complex. The conversion in JPype did not check the numpy type but rather that the size of the types matched. Thus it would accept a set of floats for ints or the opposite rather than checking if a conversion should happen first. We don't have handle all conversions but we do need to raise.

My plan was to check a table of types to see which numpy type we are dealing with. If it was a direct (int<=>numpy.int32) then it should pass directly. If it was a C++ capable cast then make it happen with a conversion routine. If not then raise an error.

We can either revert this or I can put in a new request that fixes the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment