Skip to content

Commit 5f8ef9d

Browse files
authored
Explicitly allow openArray in Tensor [], []=, fix the CI (#666)
* explicitly allow `openArray` in `[]`, `[]=` for tensors This was simply an oversight obviously * fix CI by compiling tests with `-d:ssl` * need a space, duh * use AWS mirror from PyTorch for MNIST download * fix regression caused by PR #659 By not resetting the offset here, operating on a Tensor view without cloning could cause undefined behavior, because we would be accessing elements outside the tensor buffer. * add test case for regression of #659
1 parent 9cfe757 commit 5f8ef9d

File tree

5 files changed

+56
-5
lines changed

5 files changed

+56
-5
lines changed

Diff for: arraymancer.nimble

+2-2
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,9 @@ proc test(name, switches = "", split = false, lang = "c") =
143143
if not dirExists "build":
144144
mkDir "build"
145145
if not split:
146-
exec "nim " & lang & " -o:build/" & name & switches & " -r tests/" & name & ".nim"
146+
exec "nim " & lang & " -d:ssl -o:build/" & name & switches & " -r tests/" & name & ".nim"
147147
else:
148-
exec "nim " & lang & " -o:build/" & name & switches & " -r tests/_split_tests/" & name & ".nim"
148+
exec "nim " & lang & " -d:ssl -o:build/" & name & switches & " -r tests/_split_tests/" & name & ".nim"
149149

150150
# run tests that require old RNG for backward compat. reasos
151151
let rngTests = ["spatial/test_kdtree",

Diff for: src/arraymancer/datasets/mnist.nim

+5-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,11 @@ const MNISTFilenames = [
7171
]
7272

7373
const
74-
DefaultMnistUrl = "http://yann.lecun.com/exdb/mnist/"
74+
## NOTE: As of some time before 2024/09/20 the MNIST dataset cannot be downloaded from
75+
## Yann's website anylonger, due to 403 error.
76+
## The AWS mirror here comes from PyTorch Vision:
77+
## https://github.com/pytorch/vision/blob/6d7851bd5e2bedc294e40e90532f0e375fcfee04/torchvision/datasets/mnist.py#L39C10-L39C56
78+
DefaultMnistUrl = "https://ossci-datasets.s3.amazonaws.com/mnist/" # "http://yann.lecun.com/exdb/mnist/"
7579
FashionMnistUrl = "http://fashion-mnist.s3-website.eu-central-1.amazonaws.com/"
7680

7781
proc read_mnist_images(stream: Stream): Tensor[uint8] {.noinit.} =

Diff for: src/arraymancer/tensor/private/p_accessors.nim

+2-2
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,7 @@ proc checkValidSliceType*(n: NimNode) =
496496
## the `[]` and `[]=` macros. It will raise a CT error in case it is not.
497497
##
498498
## TODO: Do we / should we allow other integer types than `tyInt` / `int`?
499-
const validTypes = {ntyInt, ntyObject, ntyArray, ntySequence, ntyGenericInst}
499+
const validTypes = {ntyInt, ntyObject, ntyArray, ntySequence, ntyOpenArray, ntyGenericInst}
500500
# `ntyObject` requires to be `Span`, ...
501501
template raiseError(arg: untyped): untyped =
502502
let typ = arg.getTypeInst
@@ -508,7 +508,7 @@ proc checkValidSliceType*(n: NimNode) =
508508
of validTypes:
509509
if arg.typeKind in {ntyObject, ntyGenericInst} and not validObjectType(arg):
510510
raiseError(arg)
511-
elif arg.typeKind in {ntyArray, ntySequence}:
511+
elif arg.typeKind in {ntyArray, ntySequence, ntyOpenArray}:
512512
# Need to check inner type!
513513
checkValidSliceType(arg.getTypeInst()[^1])
514514
break

Diff for: src/arraymancer/tensor/private/p_shapeshifting.nim

+1
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ proc reshape_no_copy*(t: AnyTensor, new_shape: varargs[int]|Metadata|seq[int], r
3838
proc reshape_with_copy*[T](t: Tensor[T], new_shape: varargs[int]|Metadata|seq[int], result: var Tensor[T]) =
3939
contiguousImpl(t, rowMajor, result)
4040
reshape_no_copy(t, new_shape, result, rowMajor)
41+
result.offset = 0 # Offset needs to be reset! Copy is a new tensor of `new_shape`
4142

4243
proc infer_shape*(t: Tensor, new_shape: varargs[int]): seq[int] {.noinit.} =
4344
## Replace the single -1 value on `new_shape` with the value that

Diff for: tests/test_bugtracker.nim

+46
Original file line numberDiff line numberDiff line change
@@ -64,5 +64,51 @@ proc main() =
6464
x.permute(1, 0) == expected
6565
x.permute(1, 0).reshape(2, 2) == expected
6666

67+
test "Test for PR #659 regression":
68+
## Data taken from `kdtree` test case, in which the node `split`
69+
## based on `data.percentile(50)` (i.e. `data.median`) suddenly
70+
## changed after PR:
71+
## https://github.com/mratsim/Arraymancer/pull/659/
72+
let t = @[@[0.195513 , 0.225253],
73+
@[0.181441 , 0.102758],
74+
@[0.26576 , 0.218074],
75+
@[0.180852 , 0.00262669],
76+
@[0.219789 , 0.191867],
77+
@[0.160581 , 0.131],
78+
@[0.269926 , 0.237261],
79+
@[0.223423 , 0.232116],
80+
@[0.191391 , 0.183001],
81+
@[0.19654 , 0.0809091],
82+
@[0.191497 , 0.0929182],
83+
@[0.22709 , 0.125705],
84+
@[0.263181 , 0.124787],
85+
@[0.204926 , 0.00688886],
86+
@[0.151998 , 0.0531739],
87+
@[0.260266 , 0.0583248],
88+
@[0.214864 , 0.110506],
89+
@[0.247688 , 0.0732228],
90+
@[0.246916 , 0.204899],
91+
@[0.215206 , 0.202225],
92+
@[0.242059 , 0.102491],
93+
@[0.159926 , 0.115765],
94+
@[0.249105 , 0.200658],
95+
@[0.195783 , 0.123984],
96+
@[0.17145 , 0.0506388],
97+
@[0.258146 , 0.0144846],
98+
@[0.215311 , 0.222503],
99+
@[0.266231 , 0.149363],
100+
@[0.178909 , 0.142174],
101+
@[0.263406 , 0.0867369],
102+
@[0.264824 , 0.221786]
103+
].toTensor
104+
105+
const exp = 0.124787
106+
let arg = t[_, 1]
107+
check arg.offset == 1 # offset of 1 due to slicing
108+
# `reshape` copies here, because `arg` is a non contiguous tensor. Thus
109+
# offset must be reset to 0
110+
check arg.reshape([1, arg.size]).offset == 0
111+
check t[_, 1].percentile(50) == exp
112+
67113
main()
68114
GC_fullCollect()

0 commit comments

Comments
 (0)