Skip to content

Conversation

alonre24
Copy link
Collaborator

@alonre24 alonre24 commented Jun 1, 2021

We store tensor of type bool as a sequence of bits, where the minimal size is one byte (as UINT8).
Add validation that we do not overflow the size of the type of the tensor when setting new tensor.

@alonre24 alonre24 requested review from DvirDukhan and lantiga June 1, 2021 16:07
@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #775 (f37e501) into master (dfc0963) will decrease coverage by 0.29%.
The diff coverage is 37.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
- Coverage   79.22%   78.92%   -0.30%     
==========================================
  Files          50       50              
  Lines        7662     7721      +59     
==========================================
+ Hits         6070     6094      +24     
- Misses       1592     1627      +35     
Impacted Files Coverage Δ
src/backends/libtflite_c/tflite_c.cpp 57.60% <0.00%> (-2.50%) ⬇️
src/backends/libtorch_c/torch_c.cpp 62.26% <0.00%> (-1.18%) ⬇️
src/backends/tensorflow.c 69.04% <0.00%> (-1.55%) ⬇️
src/backends/tflite.c 66.01% <0.00%> (ø)
src/backends/torch.c 81.71% <0.00%> (ø)
src/backends/onnxruntime.c 74.08% <11.11%> (-1.59%) ⬇️
src/redis_ai_objects/tensor.c 86.64% <89.28%> (+0.44%) ⬆️

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 993f978...f37e501. Read the comment docs.

Copy link

@DvirDukhan DvirDukhan left a comment

Choose a reason for hiding this comment

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

looks good
I think this PR should add the support for the appropriate backends also

lantiga
lantiga previously approved these changes Jun 3, 2021
Copy link
Contributor

@lantiga lantiga left a comment

Choose a reason for hiding this comment

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

Looks good!

@alonre24 alonre24 added ci-test and removed ci-test labels Jun 3, 2021
break;
case RAI_DEVICE_GPU:
dl_device = kDLGPU;
dl_device = kDLCUDA;

Choose a reason for hiding this comment

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

this is a breaking change
either introduce a new rdb encoding version or undo this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't a breaking change. dlpack renamed this in their last release: https://github.com/dmlc/dlpack/releases
but the numeric value has not changed.

@alonre24 alonre24 added ci-test and removed ci-test labels Jun 6, 2021
@alonre24 alonre24 merged commit 2f6b4be into master Jun 6, 2021
@alonre24 alonre24 deleted the Support_BOOL_type_for_tensors branch June 6, 2021 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants