add conversion form Arrow Tensor to Dense Tensor

gorgonia / tensor

package tensor provides efficient and generic n-dimensional arrays in Go that are useful for machine learning and deep learning purposes

Apache License 2.0

359 stars 49 forks source link

add conversion form Arrow Tensor to Dense Tensor #76

Closed poopoothegorilla closed 4 years ago

poopoothegorilla commented 4 years ago

This PR adds the ability to convert an Arrow Tensor to a Dense Tensor.

Notes:

[x] Add Tests
[x] Add Basic conversions for FromArrowTensor function to Dense Tensor
[x] Rename some generic Arrow vars in genlib2 to ArrowArray to keep consistency with ArrowTensor naming
[x] Update documentation for WithMask to include WithMask in example
[x] Allow a mask to be passed to the AsFortran ConsOpt
[x] Add transposeMask function for inplace and non-inplace transpose operations

coveralls commented 4 years ago

Coverage increased (+0.2%) to 73.379% when pulling 224061a0b0250d5c96d824f615fbe63495711ade on poopoothegorilla:jtw/arrow-tensor-to-gorgonia-tensor into 7d68916a621fd3fdd4393052f6c84460f610ee1b on gorgonia:master.

poopoothegorilla commented 4 years ago

Hi @cfgt , I am trying to add the null mask from the Arrow Tensor to the Dense Tensor. I am struggling with the best way to accomplish this for AsFortran or ColMajor... is there a preferred way to add support for column-major null mask? Should the AsFortran option also update the null mask if it exists?

cfgt commented 4 years ago

You should probably update AsFortran with a ...mask

poopoothegorilla commented 4 years ago

I think all should be good now. NOTE: the transposeMask method is now being called on Transpose ... we could keep it this way, but I am unsure if it might change how people are expecting the Transpose method to work. An alternative is to break it off into a separate method TransposeMask.

@cfgt what are your thoughts?

chewxy commented 4 years ago

Keep it this way. There is no reason why masked tensors should not have their masks transposed. The previous behaviour was a bug

chewxy commented 4 years ago

@poopoothegorilla if this is all good please let me know. I'd like to merge this PR

poopoothegorilla commented 4 years ago

@chewxy this is ready. Thanks for taking a look