Add Stride for Collection by ollieatkinson · Pull Request #24 · apple/swift-algorithms

ollieatkinson · 2020-10-14T15:41:35Z

Many thanks to @timvermeulen for continued help!

Stride

A type that steps over a collection’s elements by the specified amount.

This is available through the striding(by:) method on any Collection.

(0...10).striding(by: 2) // == [0, 2, 4, 6, 8, 10]

If the stride is larger than the collection count, the resulting wrapper only contains the
first element.

The stride amount must be a positive value.

Detailed Design

The striding(by:) method is declared as a Collection extension, and returns a
Stride type:

extension Collection {
  public func striding(by step: Int) -> Stride<Self>
}

A custom Index type is defined so that it's not possible to get confused when trying
to access an index of the stride collection.

[0, 1, 2, 3, 4].striding(by: 2)[1] // == 1
[0, 1, 2, 3, 4].striding(by: 2).map { $0 }[1] // == 2

A careful thought was given to the composition of these strides by giving a custom
implementation to index(_:offsetBy:limitedBy) which multiplies the offset by the
stride amount.

base.index(i.base, offsetBy: distance * stride, limitedBy: base.endIndex)

The following two lines of code are equivalent, including performance:

(0...10).striding(by: 6)
(0...10).striding(by: 2).stride(by: 3)

Complexity

The call to striding(by: k) is always O(1) and access to the next value in the stride
is O(1) if the collection conforms to RandomAccessCollection, otherwise O(k).

Comparison with other languages

rust has Strided available in a crate.
c++ has std::slice::stride

The semantics of striding described in this documentation are equivalent.

Checklist

I've added at least one test that validates that my change is working, if appropriate
I've followed the code style of the rest of the project
I've read the Contribution Guidelines
I've updated the documentation if necessary

xwu · 2020-10-14T22:00:28Z

+  /// - Parameter step: The amount to step with each iteration.
+  /// - Returns: Returns a collection stepping through the elements by the
+  /// specified amount.
+  public func stride(by step: Int) -> Stride<Self> {


I wonder if this would best be striding to fit the ed/ing rule for naming nonmutating members. That said, I also worry about ambiguity with the existing StrideTo/StrideThrough types: are there other terms that are often used for this operation?

I am in agreement that striding is probably a better fit for this name for the reasons you've mentioned. I'm a little unsure that renaming the type to something else would be a good idea as the notion of a stride is widely known and something which is obvious as soon as you see it. Other terms used for striding include increment, pitch or step - whilst they offer a similar meaning, they do not have the same understanding as striding. In one of my earlier implementations I had named the type StridingCollection which I would not be against being explicit about, I removed the suffix Collection to keep in step with the rest of the algorithms in this repository. It would be good to see what other feedback I get regarding the naming of this before I change it. Thanks for your thoughts @xwu

natecook1000

This is looking good, @ollieatkinson — thanks for this addition!

natecook1000

@ollieatkinson A few more notes for you!

xwu · 2020-10-20T23:12:15Z

+
+public struct Stride<Base: Collection> {
+
+  public let base: Base


To help simplify the issue, do we need to expose base at all?

I was also unsure of the value of being able to refer to base in this instance, but since all of the other collection algorithms do I thought it would be worth keeping consistent. Was there a specific issue you think that it might simplify?

Sorry, the specific issue is the question of equality, which @natecook1000 mentioned above.

Two values are equal when they behave the same way for all salient operations. For this purpose, we say that iteration order isn’t salient for sets but is salient for arrays, that the sign of zero isn’t salient for floating-point numbers, etc.

It’s a judgment call about what is salient here, and reasonable people can disagree whether differing base elements would matter; by contrast, if base isn’t recoverable, then there is no ambiguity at all.

I like what you're saying here and I'd be up for dropping the access modifier from the property - I cannot fathom a situation where you would want to use it outside of the scope of the collection itself.

Agreed on this — base can be internal, which removes any controversy here. (That should probably be the case for the rest of the collection wrappers as well.)

Thanks both for your thoughts, I've removed public from base here and we can follow up in another P/R for the rest of the collections

by contrast, if base isn’t recoverable, then there is no ambiguity at all.

I don't see how this matters in case the user knows what base is, even if Stride doesn't expose it. Couldn't there still be controversy about whether (0...10).striding(by: 4) should equal (0...11).striding(by: 4)?

natecook1000

I think just these last couple of notes, then this will be ready 👍

natecook1000 · 2020-10-27T16:20:45Z

+  }
+
+  public func index(_ i: Index, offsetBy distance: Int) -> Index {
+    precondition(i.base < base.endIndex, "Advancing past end index")


This isn't a valid precondition, since c.index(c.endIndex, offsetBy: -1) is valid for bidirectional collections.

…lities for validateIndexTraversals

ollieatkinson · 2020-11-01T20:32:08Z

I've rebased against the latest main and included some test cases using the new validateIndexTraversals TestUtilities API. Before merging, it would be good to hear your thoughts on the points that xwu raised here

timvermeulen · 2020-11-05T14:35:58Z

Stride probably shouldn't require that Base: Collection, but instead implement only Sequence when Base: Sequence.

ollieatkinson · 2020-11-06T09:59:25Z

@timvermeulen

Stride probably shouldn't require that Base: Collection, but instead implement only Sequence when Base: Sequence.

That makes sense, I've added a commit to address this behaviour here

ollieatkinson · 2020-11-20T17:42:59Z

@natecook1000 Is there any more work required on this for it to be merged?

jshier · 2020-11-29T03:19:27Z

+    limitedBy limit: Index
+  ) -> Index? {
+    let distance = i == endIndex
+      ? -((base.count - 1) % stride + 1) + (n - 1) * -stride


FYI, Xcode 12.2 fails to compile this line due to an "expression too complex" error. Making the 1 values Int(1) seems to resolve the issue.

Thanks for raising this - I'm surprised that it's too complex given these are all integers, I could also annotate the distance property : Int to have the same effect.

I have just tested this on Big Sur 11.0.1 and Xcode 12.2 and I was able to compile fine - do you have anything else in your environment which might cause this, so I can track it down?

natecook1000 · 2020-12-01T23:53:50Z

Thanks, @ollieatkinson — this is ready to go!

natecook1000 reviewed Oct 14, 2020

View reviewed changes

Comment thread Sources/Algorithms/Stride.swift Outdated

ollieatkinson marked this pull request as draft October 14, 2020 16:20

timvermeulen reviewed Oct 14, 2020

View reviewed changes

Comment thread Sources/Algorithms/Stride.swift Outdated

ollieatkinson force-pushed the striding-collection branch from 6819ce7 to 18bd60d Compare October 14, 2020 20:28

ollieatkinson marked this pull request as ready for review October 14, 2020 20:30

xwu reviewed Oct 14, 2020

View reviewed changes

natecook1000 requested changes Oct 20, 2020

View reviewed changes

natecook1000 reviewed Oct 20, 2020

View reviewed changes

xwu reviewed Oct 20, 2020

View reviewed changes

natecook1000 reviewed Oct 27, 2020

View reviewed changes

ollieatkinson force-pushed the striding-collection branch from afbb684 to 74a25e8 Compare November 1, 2020 20:28

ollieatkinson added 2 commits November 1, 2020 20:30

Add Stride for Collection

17cb07e

Refactor to index(_:offsetBy:limitedBy:) and utilise the new test uti…

6df6133

…lities for validateIndexTraversals

ollieatkinson force-pushed the striding-collection branch from 74a25e8 to 6df6133 Compare November 1, 2020 20:30

ollieatkinson added 2 commits November 1, 2020 20:39

remove redundant limit

4a194ca

make base and stride internal

dcdef50

timvermeulen reviewed Nov 5, 2020

View reviewed changes

Implementation of feedback from @timvermeulen

6f7148d

ollieatkinson added 3 commits November 6, 2020 10:13

Add implementation for Sequence iterator

28bbed8

small documentation tweek collection -> sequence

377bb72

Add unbounded striding tests to check empty and sequence implementation

92b2846

timvermeulen reviewed Nov 6, 2020

View reviewed changes

Comment thread Sources/Algorithms/Stride.swift

compute the stride only when the next element is requested

0a6bcc3

timvermeulen reviewed Nov 6, 2020

View reviewed changes

Comment thread Sources/Algorithms/Stride.swift Outdated

Comment thread Sources/Algorithms/Stride.swift Outdated

offsetBackward n is positive and remove init? from Index

3669ff0

ollieatkinson force-pushed the striding-collection branch from f8461f5 to 3669ff0 Compare November 6, 2020 15:50

consistant negative stride

dfcc611

jshier reviewed Nov 29, 2020

View reviewed changes

natecook1000 approved these changes Dec 1, 2020

View reviewed changes

natecook1000 merged commit ff01a02 into apple:main Dec 1, 2020

ollieatkinson deleted the striding-collection branch December 2, 2020 09:01


		public struct Stride<Base: Collection> {

		public let base: Base

Conversation

ollieatkinson commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stride

Detailed Design

Complexity

Comparison with other languages

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ollieatkinson Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ollieatkinson Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ollieatkinson commented Nov 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timvermeulen commented Nov 5, 2020

Uh oh!

ollieatkinson commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ollieatkinson commented Nov 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ollieatkinson Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ollieatkinson commented Oct 14, 2020 •

edited

Loading

ollieatkinson Oct 15, 2020 •

edited

Loading

ollieatkinson Nov 2, 2020 •

edited

Loading

ollieatkinson commented Nov 6, 2020 •

edited

Loading

ollieatkinson Nov 29, 2020 •

edited

Loading