Split #16

KhalidAl-otaibi · 2023-04-21T04:30:21Z

Split function and other basic function such as expandingShape and squeezingShape. I think there is a much better implementation for the split function but this is what I came up with. It uses a few helper functions. These helper functions could also be used for subscription. In fact, if subscription was implemented, split could have been implemented very straightforwardly.

…se of split.

rexmas · 2023-04-24T01:20:19Z

if subscription was implemented

Not sure if you mean subscript or something else, but if you mean subscript we do have an impl for that

swift-shaped-array/Sources/ShapedArray/ShapedArray.swift

Line 122 in 8cb542c

public subscript(index: Int) -> Element {

rexmas · 2023-04-24T01:43:11Z

Sources/ShapedArray/Basic.swift

+    /// takes a shape for the original array, a shape for the subarray, 
+    /// and an axis on which the original array would be divided 
+    /// and returns an array of start and end indices for each subarray
+


For doc comments, let's use both starting capitalization and ending punctuation. also // helper function is missing an /

rexmas · 2023-04-24T01:51:28Z

Sources/ShapedArray/Basic.swift

+    /// and returns an array of start and end indices for each subarray
+
+    @usableFromInline
+    internal func _calculateSubarrayIndices(shape: [Int], subarrayShape: [Int], axis: Int) -> [SubarrayIndices] {


The only use of the_ prefix for a name is for public identifiers such as public protocol _ShapedArrayProtocol because they need to be public to work yet we want to signal to the user that they shouldn't be used directly. For non-public identifiers I think we should omit _, in this case that would be calculateSubarrayIndices.

However, i think we can improve on the name more too. Possibly

slicedSubArrayIndices(forShape shape: [Int], subarrayShape: [Int], alongAxis axis: Int) -> [SubarrayIndices]

?

Lastly, and maybe this isn't possible with @usableFromInline, but if this is only used in 1 function I usually move the function declaration inside of the function that uses it. It adds some indentation but makes it clear at point of use that it isn't used elsewhere. Up to you though, nbd either way.

or maybe splitSubArrayIndices to match the calling function..

rexmas · 2023-04-24T01:53:26Z

Sources/ShapedArray/Basic.swift

+    internal func _calculateSubarrayIndices(shape: [Int], subarrayShape: [Int], axis: Int) -> [SubarrayIndices] {
+        let numberOfSubarrays = shape[axis] / subarrayShape[axis]
+
+        var subarrays = [SubarrayIndices]()


Trying to think of a name that better captures this type, maybe var groupedSubarrayIndices? any suggestions?

slicedSubarrayIndices?

rexmas · 2023-04-24T01:56:45Z

Sources/ShapedArray/Basic.swift

+            endIndices[axis] = startIndices[axis] + subarrayShape[axis]
+
+            subarrays.append(SubarrayIndices(start: startIndices, end: endIndices))
+        }


this may look a bit nicer with a map

let subarrays = (0..<numberSubarrays).map { i in var startIndices = [Int](repeating: 0, count: shape.count) var endIndices = shape startIndices[axis] = i * subarrayShape[axis] endIndices[axis] = startIndices[axis] + subarrayShape[axis] return SubarrayIndices(start: startIndices, end: endIndices) }

rexmas · 2023-04-24T01:57:02Z

Sources/ShapedArray/Basic.swift

+    }
+
+    // helper function for subarray indices generation
+    /// takes a start and end index for each dimension and returns all the n-dim indices in between


Same comment as before about doc comments.

rexmas · 2023-04-24T01:57:40Z

Sources/ShapedArray/Basic.swift

+    /// Splits the ShapedArray into multiple subarrays along the given axis.
+    /// - Parameters:
+    ///   - count: The number of subarrays to return.
+    ///  - axis: The axis along which to split the ShapedArray. Negative values wrap around.


- axis seems misaligned with - count

rexmas · 2023-04-24T02:23:06Z

Sources/ShapedArray/Basic.swift

+    // helper function for subarray indices generation
+    /// takes a start and end index for each dimension and returns all the n-dim indices in between
+    @usableFromInline
+    internal func _generateIndices(start: [Int], end: [Int], currentIndex: [Int] = [], depth: Int = 0) -> [[Int]] {


Maybe

allIndices(fromStart start: [Int], toEnd end: [Int], currentIndices: [Int] = [], depth: Int = 0) -> [[Int]]

rexmas · 2023-04-24T02:24:24Z

Sources/ShapedArray/Basic.swift

+
+        for i in start[depth]..<end[depth] {
+            let newIndex = currentIndex + [i]
+            let subIndices = _generateIndices(start: start, end: end, currentIndex: newIndex, depth: depth + 1)


This isn't tail recursive so there's no tail-call optimization. Granted probably plenty to optimize in the future, but maybe add a comment here calling that out explicitly?
E.g. TODO: Possible tail-call optimization?

rexmas · 2023-04-24T02:24:36Z

Sources/ShapedArray/Basic.swift

+    }
+
+    // helper function for subarray indices generation
+    /// takes a shape, strides, and n-dim indices and returns the linear index


rexmas · 2023-04-24T02:27:29Z

Sources/ShapedArray/Basic.swift

+    // helper function for subarray indices generation
+    /// takes a shape, strides, and n-dim indices and returns the linear index
+    @usableFromInline
+    internal func _calculateLinearIndex(shape: [Int], strides: [Int], indices: [Int]) -> Int {


Maybe

linearIndexFrom(shape: [Int], strides: [Int], indices: [Int]) -> Int

?
other option may be bufferIndexFrom(shape:strides:indices)

rexmas · 2023-04-24T02:27:50Z

Sources/ShapedArray/Basic.swift

+            }
+            linearIndex += stride * index
+        }
+    return linearIndex


indentation is very off here...

rexmas · 2023-04-24T02:28:05Z

Sources/ShapedArray/Basic.swift

+    return linearIndex
+    }
+
+


only 1 newline please

rexmas · 2023-04-24T02:28:10Z

Sources/ShapedArray/Basic.swift

+        return newArrays
+    }
+
+


only 1 newline please

rexmas · 2023-04-24T02:28:36Z

Sources/ShapedArray/Basic.swift

@@ -70,15 +183,17 @@ extension ShapedArray {
    /// - Precondition: `axis` must be in the range `[-rank, rank)`, where `rank` is the rank of
    ///   the provided ShapedArrays.
    ///
+
+


both newlines here should be deleted please, this breaks up the comment

rexmas · 2023-04-24T02:29:48Z

Sources/ShapedArray/ShapedArrayProtocol.swift

@@ -318,3 +318,14 @@ extension _ShapedArrayProtocol where Scalar: Equatable {
        }
    }
 }
+
+extension _ShapedArrayProtocol where Scalar: Comparable {
+    internal func _isLess(than other: Self) -> Bool {


I don't see this function used anywhere, is this something we need to keep?

rexmas · 2023-04-24T02:31:37Z

Tests/ShapedArrayTests/ShapedArrayTests.swift

+
+    }
+
+    // test expandingShape


Comment here probably not needed since it's in the name of the test function

rexmas · 2023-04-24T02:36:46Z

Sources/ShapedArray/Basic.swift

@@ -99,6 +214,47 @@ extension ShapedArray {

        return newArrays.map { ShapedArray(shape: newShape, scalars: $0) }
    }
+
+    @inlinable


Some docs we can include:

/// Returns a shape-expanded `ShapedArray`, with a dimension of 1 inserted at the specified shape /// indices.

rexmas · 2023-04-24T02:37:22Z

Sources/ShapedArray/Basic.swift

+    }
+
+    @inlinable
+    public func rankLifted() -> ShapedArray {


Possible docs for this:

/// Returns a rank-lifted `ShapedArray` with a leading dimension of 1.

rexmas

thank you for this! left some comments before approval, but nothing major

KhalidAl-otaibi added 2 commits March 26, 2023 21:59

Added Split and refactored unstacked to use it, as it is a special ca…

3ccc12d

…se of split.

split function and other basic functions

3752884

KhalidAl-otaibi requested review from rexmas and JaapWijnen April 21, 2023 04:30

rexmas reviewed Apr 24, 2023

View reviewed changes

Sources/ShapedArray/Basic.swift

}

linearIndex += stride * index

}

return linearIndex

Copy link

Member

rexmas Apr 24, 2023

Choose a reason for hiding this comment

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

indentation is very off here...

rexmas reviewed Apr 24, 2023

View reviewed changes

Sources/ShapedArray/Basic.swift

return linearIndex

}

Copy link

Member

rexmas Apr 24, 2023

Choose a reason for hiding this comment

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

only 1 newline please

rexmas reviewed Apr 24, 2023

View reviewed changes

Sources/ShapedArray/Basic.swift

return newArrays

}

Copy link

Member

rexmas Apr 24, 2023

Choose a reason for hiding this comment

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

only 1 newline please

rexmas reviewed Apr 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split #16

Split #16

KhalidAl-otaibi commented Apr 21, 2023

rexmas commented Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas Apr 24, 2023

rexmas left a comment

Split #16

Are you sure you want to change the base?

Split #16

Conversation

KhalidAl-otaibi commented Apr 21, 2023

rexmas commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rexmas left a comment

Choose a reason for hiding this comment