Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Dense tensor definition #8

Closed
wants to merge 1 commit into from

Conversation

elferherrera
Copy link
Contributor

Initial definition of dense tensor. It includes constructor and functions to interact with it

@codecov-io
Copy link

Codecov Report

Merging #8 (f1722ed) into main (aa2033e) will increase coverage by 0.40%.
The diff coverage is 88.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
+ Coverage   62.40%   62.81%   +0.40%     
==========================================
  Files         143      146       +3     
  Lines       13977    14287     +310     
==========================================
+ Hits         8722     8974     +252     
- Misses       5255     5313      +58     
Impacted Files Coverage Δ
src/tensor/dense.rs 88.17% <88.17%> (ø)
src/compute/sort/mod.rs 52.27% <0.00%> (-17.05%) ⬇️
src/array/boolean/iterator.rs 50.00% <0.00%> (-14.00%) ⬇️
src/compute/take/primitive.rs 94.33% <0.00%> (-5.67%) ⬇️
src/array/ord.rs 63.94% <0.00%> (-4.09%) ⬇️
src/compute/take/mod.rs 78.94% <0.00%> (-1.06%) ⬇️
src/bits/zip_validity.rs 97.70% <0.00%> (-0.99%) ⬇️
src/buffer/mutable.rs 89.41% <0.00%> (ø)
src/compute/sort/lex_sort.rs 83.13% <0.00%> (ø)
src/array/primitive/iterator.rs 50.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa2033e...f1722ed. Read the comment docs.

@jorgecarleitao
Copy link
Owner

jorgecarleitao commented Mar 14, 2021

Hi @elferherrera , thanks a lot for this!

I looked it up for integration files to perform integration tests, but I have not found any. I can see that the arrow crate itself also has no integration tests for tensors, which suggests that this feature is not well maintained.

A concern I have is that we would offer tensor support, but then have no process of verifying its compatibility against other implementations, which IMO is the hallmark of the arrow format. It is true that the arrow crate itself has support, but without integration tests, there is no proof that what it is done there is in spec, right?

Do you have an idea on how we should handle this?

@elferherrera
Copy link
Contributor Author

Hi. Actually that is something I want to write about in the mailing list. I know they have dense and sparse tensor definitions in C++ implementation but I couldn't find how they are testing it. I have an idea of how to add it to the new IPC format you have but I was wondering how we can test it with the C++ side.

I guess it will have to be a joint effort with the C++ team, unless they are not interested in this feature.

@elferherrera
Copy link
Contributor Author

elferherrera commented Mar 19, 2021

What do you think we should do with these tensors?. I would like to write more implementations for dense and sparse tensors, especially to be able to use them with IPC. However, I don't think they will be consider them soon into main Arrow.

I was wondering that it could be a great idea to create an implementation of tensor that can create an ndarray.

@jorgecarleitao
Copy link
Owner

Firstly, thanks a lot for driving that in the mailing list..

I think that we should not have added them to the master arrow crate in the first place, as there is no guarantee that they can be used throughout the ecosystem. IMO we should not invest time on them without IPC tests, as we risk drifting from the specification, which IMO is detrimental to the arrow project as a whole.

I think that the path forward in this direction is to write a google docs describing the json-integration for tensors and share them on the mailing list, so that folks from C++ and the spec itself can take a look and see whether the format would make sense. Note that these are very simple files in json. E.g. the validity is just "true" / "false".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants