From 3e7dce668ad33f3d7ac8899afa10dde93c5e64ef Mon Sep 17 00:00:00 2001 From: Vindaar Date: Fri, 20 Sep 2024 12:28:17 +0200 Subject: [PATCH 1/6] explicitly allow `openArray` in `[]`, `[]=` for tensors This was simply an oversight obviously --- src/arraymancer/tensor/private/p_accessors.nim | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/arraymancer/tensor/private/p_accessors.nim b/src/arraymancer/tensor/private/p_accessors.nim index fc868e17..837012a3 100644 --- a/src/arraymancer/tensor/private/p_accessors.nim +++ b/src/arraymancer/tensor/private/p_accessors.nim @@ -496,7 +496,7 @@ proc checkValidSliceType*(n: NimNode) = ## the `[]` and `[]=` macros. It will raise a CT error in case it is not. ## ## TODO: Do we / should we allow other integer types than `tyInt` / `int`? - const validTypes = {ntyInt, ntyObject, ntyArray, ntySequence, ntyGenericInst} + const validTypes = {ntyInt, ntyObject, ntyArray, ntySequence, ntyOpenArray, ntyGenericInst} # `ntyObject` requires to be `Span`, ... template raiseError(arg: untyped): untyped = let typ = arg.getTypeInst @@ -508,7 +508,7 @@ proc checkValidSliceType*(n: NimNode) = of validTypes: if arg.typeKind in {ntyObject, ntyGenericInst} and not validObjectType(arg): raiseError(arg) - elif arg.typeKind in {ntyArray, ntySequence}: + elif arg.typeKind in {ntyArray, ntySequence, ntyOpenArray}: # Need to check inner type! checkValidSliceType(arg.getTypeInst()[^1]) break From 836c6b25da5747b46c7ea0063650b9bd0bc63bb0 Mon Sep 17 00:00:00 2001 From: Vindaar Date: Fri, 20 Sep 2024 12:32:05 +0200 Subject: [PATCH 2/6] fix CI by compiling tests with `-d:ssl` --- arraymancer.nimble | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arraymancer.nimble b/arraymancer.nimble index c568ba92..8293011e 100644 --- a/arraymancer.nimble +++ b/arraymancer.nimble @@ -143,9 +143,9 @@ proc test(name, switches = "", split = false, lang = "c") = if not dirExists "build": mkDir "build" if not split: - exec "nim " & lang & " -o:build/" & name & switches & " -r tests/" & name & ".nim" + exec "nim " & lang & "-d:ssl -o:build/" & name & switches & " -r tests/" & name & ".nim" else: - exec "nim " & lang & " -o:build/" & name & switches & " -r tests/_split_tests/" & name & ".nim" + exec "nim " & lang & "-d:ssl -o:build/" & name & switches & " -r tests/_split_tests/" & name & ".nim" # run tests that require old RNG for backward compat. reasos let rngTests = ["spatial/test_kdtree", From 7a93d3858176e27ab0b1fe3ef39f0814b7f9d4c4 Mon Sep 17 00:00:00 2001 From: Vindaar Date: Fri, 20 Sep 2024 12:35:05 +0200 Subject: [PATCH 3/6] need a space, duh --- arraymancer.nimble | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arraymancer.nimble b/arraymancer.nimble index 8293011e..09ea5e65 100644 --- a/arraymancer.nimble +++ b/arraymancer.nimble @@ -143,9 +143,9 @@ proc test(name, switches = "", split = false, lang = "c") = if not dirExists "build": mkDir "build" if not split: - exec "nim " & lang & "-d:ssl -o:build/" & name & switches & " -r tests/" & name & ".nim" + exec "nim " & lang & " -d:ssl -o:build/" & name & switches & " -r tests/" & name & ".nim" else: - exec "nim " & lang & "-d:ssl -o:build/" & name & switches & " -r tests/_split_tests/" & name & ".nim" + exec "nim " & lang & " -d:ssl -o:build/" & name & switches & " -r tests/_split_tests/" & name & ".nim" # run tests that require old RNG for backward compat. reasos let rngTests = ["spatial/test_kdtree", From 5a28ccfd08eece449276073cbc498a6c63c33906 Mon Sep 17 00:00:00 2001 From: Vindaar Date: Fri, 20 Sep 2024 12:58:05 +0200 Subject: [PATCH 4/6] use AWS mirror from PyTorch for MNIST download --- src/arraymancer/datasets/mnist.nim | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/arraymancer/datasets/mnist.nim b/src/arraymancer/datasets/mnist.nim index eda99ebf..1366ccd7 100644 --- a/src/arraymancer/datasets/mnist.nim +++ b/src/arraymancer/datasets/mnist.nim @@ -71,7 +71,11 @@ const MNISTFilenames = [ ] const - DefaultMnistUrl = "http://yann.lecun.com/exdb/mnist/" + ## NOTE: As of some time before 2024/09/20 the MNIST dataset cannot be downloaded from + ## Yann's website anylonger, due to 403 error. + ## The AWS mirror here comes from PyTorch Vision: + ## https://github.com/pytorch/vision/blob/6d7851bd5e2bedc294e40e90532f0e375fcfee04/torchvision/datasets/mnist.py#L39C10-L39C56 + DefaultMnistUrl = "https://ossci-datasets.s3.amazonaws.com/mnist/" # "http://yann.lecun.com/exdb/mnist/" FashionMnistUrl = "http://fashion-mnist.s3-website.eu-central-1.amazonaws.com/" proc read_mnist_images(stream: Stream): Tensor[uint8] {.noinit.} = From 93a6506736300a3b0f52094a20c0af85bcfa68ea Mon Sep 17 00:00:00 2001 From: Vindaar Date: Fri, 20 Sep 2024 17:35:02 +0200 Subject: [PATCH 5/6] 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. --- src/arraymancer/tensor/private/p_shapeshifting.nim | 1 + 1 file changed, 1 insertion(+) diff --git a/src/arraymancer/tensor/private/p_shapeshifting.nim b/src/arraymancer/tensor/private/p_shapeshifting.nim index 840b1c8e..0844a60c 100644 --- a/src/arraymancer/tensor/private/p_shapeshifting.nim +++ b/src/arraymancer/tensor/private/p_shapeshifting.nim @@ -38,6 +38,7 @@ proc reshape_no_copy*(t: AnyTensor, new_shape: varargs[int]|Metadata|seq[int], r proc reshape_with_copy*[T](t: Tensor[T], new_shape: varargs[int]|Metadata|seq[int], result: var Tensor[T]) = contiguousImpl(t, rowMajor, result) reshape_no_copy(t, new_shape, result, rowMajor) + result.offset = 0 # Offset needs to be reset! Copy is a new tensor of `new_shape` proc infer_shape*(t: Tensor, new_shape: varargs[int]): seq[int] {.noinit.} = ## Replace the single -1 value on `new_shape` with the value that From 46c1206153eb8bbe700a03c660f39d96a975bce7 Mon Sep 17 00:00:00 2001 From: Vindaar Date: Fri, 20 Sep 2024 18:04:51 +0200 Subject: [PATCH 6/6] add test case for regression of #659 --- tests/test_bugtracker.nim | 46 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/test_bugtracker.nim b/tests/test_bugtracker.nim index 4b423d87..c295bb3a 100644 --- a/tests/test_bugtracker.nim +++ b/tests/test_bugtracker.nim @@ -64,5 +64,51 @@ proc main() = x.permute(1, 0) == expected x.permute(1, 0).reshape(2, 2) == expected + test "Test for PR #659 regression": + ## Data taken from `kdtree` test case, in which the node `split` + ## based on `data.percentile(50)` (i.e. `data.median`) suddenly + ## changed after PR: + ## https://github.com/mratsim/Arraymancer/pull/659/ + let t = @[@[0.195513 , 0.225253], + @[0.181441 , 0.102758], + @[0.26576 , 0.218074], + @[0.180852 , 0.00262669], + @[0.219789 , 0.191867], + @[0.160581 , 0.131], + @[0.269926 , 0.237261], + @[0.223423 , 0.232116], + @[0.191391 , 0.183001], + @[0.19654 , 0.0809091], + @[0.191497 , 0.0929182], + @[0.22709 , 0.125705], + @[0.263181 , 0.124787], + @[0.204926 , 0.00688886], + @[0.151998 , 0.0531739], + @[0.260266 , 0.0583248], + @[0.214864 , 0.110506], + @[0.247688 , 0.0732228], + @[0.246916 , 0.204899], + @[0.215206 , 0.202225], + @[0.242059 , 0.102491], + @[0.159926 , 0.115765], + @[0.249105 , 0.200658], + @[0.195783 , 0.123984], + @[0.17145 , 0.0506388], + @[0.258146 , 0.0144846], + @[0.215311 , 0.222503], + @[0.266231 , 0.149363], + @[0.178909 , 0.142174], + @[0.263406 , 0.0867369], + @[0.264824 , 0.221786] + ].toTensor + + const exp = 0.124787 + let arg = t[_, 1] + check arg.offset == 1 # offset of 1 due to slicing + # `reshape` copies here, because `arg` is a non contiguous tensor. Thus + # offset must be reset to 0 + check arg.reshape([1, arg.size]).offset == 0 + check t[_, 1].percentile(50) == exp + main() GC_fullCollect()