Add reductions(_:_:) #46

danielctull · 2020-11-24T13:24:45Z

Description

This addition allows the caller to get a sequence of the results of reduce applied to up to and including the element of a sequence.

This adds an implementation for reductions as discussed in issue #25.

Detailed Design

To achieve this, a new sequence type called Reductions is added to the library.

For ease of use, a new method is added to sequence to provide the reductions:

extension Sequence {
  public func reductions<Result>(
    _ initial: Result,
    _ transform: @escaping (Result, Element) -> Result
) -> Reductions<Result, Self>

The implementation of subscript(position:) calls reduce each time, so there's likely some improvements to be made around the Collection conformance.

Documentation Plan

As the reductions(_:_:) method is the main entry point, this is documented with an example of use. The Reductions sequence type itself is lightly documented, which I believe follows the trend of the current APIs in the library.

In the guide, I have also gathered some quotes to show why the name reductions was chosen over scan.

Test Plan

Arrays and sequences are used to test each code path, calling reductions(_:_:) on each. The empty sequence is also tested to ensure it yields an empty sequence.

Source Impact

This is a purely additive change.

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

CTMacUser · 2020-11-25T06:42:27Z

I think you should make this an eager/lazy pair of methods. The eager version returns an Array, and may use a throwing function for the combining closure. The lazy version uses a non-throwing, but escaping function instead for its combing closure. (Some of pull requests I've recently made show how you can share code between versions.)

But the important part is that this facility cannot be a collection, only a sequence, since dereferencing imposes an O(n) time penalty.

danielctull · 2020-11-25T18:04:46Z

All similar functions in other languages that I'm aware of include the initial value in the output (except for the reactive ones). Maybe we should too?

I was under the impression that others don't include the initial value. I think C++ has the ability to do either with an inclusive and exclusive variants – but I'm not a C++ person, I was just going on what I could make of their documentation.

Most of my uses in stuff like the advent of code puzzles generally require the initial value though. In drafting this, I had another function which used chain to prepend the initial value. I would love if we had both options easily available, and am open to options with respect to name. One idea I had was reductions(including:_:) and reductions(excluding:_:), but I wasn't sure if this was making the excluding case too verbose?

But the important part is that this facility cannot be a collection, only a sequence, since dereferencing imposes an O(n) time penalty.

Subscripting indeed needs to be done in constant time, but that doesn't mean that Reductions can't be a collection. If the index stores the base index as well as the accumulated value up until that point, you can satisfy all of Collection's performance requirements. BidirectionalCollection is definitely off the table, though.

Yep, after @CTMacUser's comment I went and took a look at the implementation. I got stuck with index(_:offsetBy:) for negative offsets, it feels like this hits the same problem as BidirectionalCollection where the reductions up to the given index have to be built up again.

timvermeulen · 2020-11-25T22:24:07Z

Edit: I accidentally deleted my previous comment, but thankfully you quoted the entire thing 🙂

I was under the impression that others don't include the initial value. I think C++ has the ability to do either with an inclusive and exclusive variants – but I'm not a C++ person, I was just going on what I could make of their documentation.

From what I can see, the exclusive variant takes an explicit initial value which is included in the result, and the inclusive variant uses the first element of the iterator as the initial value, which is then also included in the result. The initial value is included in the result either way, and the functions I checked in other languages seem to behave in the same way.

I would love if we had both options easily available

It seems reasonable to me to have a variant of reductions that takes no initial value, i.e.

[1, 2, 3].reductions(+) // [1, 3, 6]

reduce doesn't have such a variant, but that probably doesn't need to stop us here.

Speaking of reduce, should reductions(into:_:) be included too? I believe it could benefit from the same copy-on-write gains that reduce(into:_:) was added for, as long as the user doesn't keep the partial result around when the next one is computed.

I got stuck with index(_:offsetBy:) for negative offsets, it feels like this hits the same problem as BidirectionalCollection where the reductions up to the given index have to be built up again.

Without BidirectionalConformance, index(_:offsetBy:) doesn't need to be able to handle negative distances. But since the elements are always going to applied one by one, there isn't much point to override this method in the first place because you won't be able to improve on the default implementation.

CTMacUser · 2020-11-26T07:57:01Z

The reactive version of scan from Apple's Combine library does not vend the initial seed, although neither the documentation nor included example make that clear.

timvermeulen · 2020-11-26T09:30:16Z

Apparently ReactiveX does not output the seed either, except for RxJava 😅 But I have yet to come across a non-reactive scan that does not output the seed.

pyrtsa · 2020-11-26T09:47:40Z

For a non-reactive stream, I don't think it makes sense to discard the seed because it's so easy to skip by calling dropFirst() on the result. And the seed value is good to keep because it is often useful.

Defining reductions to include its seed value also aligns with the way sequence(first:next:) is defined in the standard library.

Off-topic: For reactive streams if the result is also a stream of events, it probably makes sense to have the result of scan to match the length, and emission times, of the stream of incoming events. For those, the instant when to emit the seed would otherwise be left a bit ambiguous. The situation would be different if instead of a stream of discrete events the resulting value would be a continuous-time defined value (sometimes known as a behaviour). In that case it would start off immediately at the seed value and change to successive values right after every incoming event of inputs. I think we should view the result of reductions like that: it's the cumulative values for which the input sequence is the number of changes (and thus one less in length).

danielctull · 2020-11-26T12:22:33Z

These latest commits improve the implementation of the Collection conformance, remove BidirectionalCollection conformance and add two eager variants: one that takes no initial value and another which outputs the initial value at the start of the returned array. I realise lazy versions of these should also be created as well, but would rather we made sure what we wanted from the API before changing that part too. 🙂

In its current state the API looks like the following:

[1, 2, 3].reductions(excluding: 10, +) // [11, 13, 16]
[1, 2, 3].reductions(including: 10, +) // [10, 11, 13, 16]
[1, 2, 3].reductions(+) // [1, 3, 6]

I quite like @pyrtsa's point that if reductions included the provided initial value by default and the first API was removed, it is easy to produce the functionality of that removed function by calling dropFirst() on the result, keeping the reductions API cleaner. If this was the consensus, the eager API would be the following, with matching lazy versions.

[1, 2, 3].reductions(10, +) // [10, 11, 13, 16]
[1, 2, 3].reductions(+) // [1, 3, 6]

pyrtsa · 2020-11-26T12:26:48Z

I agree this would be ideal:

[1, 2, 3].reductions(10, +) // [10, 11, 13, 16]
[1, 2, 3].reductions(+) // [1, 3, 6]

timvermeulen · 2020-11-26T13:27:07Z

And potentially:

[1, 2, 3].reductions(into: 10, +=) // [10, 11, 13, 16]

reductions(_:_:) can always just call reductions(into:_:), but the variant without a seed does probably need different internals.

danielctull · 2020-11-26T13:30:18Z

And potentially:

[1, 2, 3].reductions(into: 10, +=) // [10, 11, 13, 16]

Yup! I was trying to summarise the comments so far and I failed, but I agree this should be added too. 🙂

pyrtsa · 2020-11-26T13:38:58Z

While it might be nice for symmetry, I don't see the eager reductions(into:_:) as having such a strong point for existence as reduce(into:_:) originally did, because its implementation is nevertheless going to make N modified copies in addition to the original seed.

The lazy version might be good though (because iterating over it could benefit from copy-on-write), and maybe the eager one indeed just for symmetry.

timvermeulen · 2020-11-26T13:43:53Z

The eager one can still have ergonomic advantages! Just no performance ones 🙂

CTMacUser · 2020-11-26T18:17:00Z

Eager versions can also use throwing closures.

odmir · 2020-11-26T20:11:42Z

let a = [1, 2, 3, 4].lazy.prefix(2).map { $0 }
let b = [1, 2, 3, 4].lazy.reductions(0, +).map { $0 }

print(type(of: a))
// LazyMapSequence<Slice<LazySequence<Array<Int>>>, Int>
print(type(of: b))
// Array<Int>

The current implementation loses the "laziness" information requiring one to apply .lazy again to the result.

Sources/Algorithms/Reductions.swift

danielctull · 2020-11-26T20:27:01Z

The current implementation loses the "laziness" information requiring one to apply .lazy again to the result.

Thanks @odmir, the conditional conformances to LazySequenceProtocol and LazyCollectionProtocol was missing. Using your example now yields the following. 🙂

let a = [1, 2, 3, 4].lazy.prefix(2).map { $0 }
let b = [1, 2, 3, 4].lazy.reductions(0, +).map { $0 }

print(type(of: a))
// LazyMapSequence<Slice<LazySequence<Array<Int>>>, Int>
print(type(of: b))
// LazyMapSequence<Reductions<Int, LazySequence<Array<Int>>>, Int>

timvermeulen · 2020-11-26T20:36:36Z

Sources/Algorithms/Reductions.swift

+  public func reductions(
+    _ transform: (Element, Element) throws -> Element
+  ) rethrows -> [Element] {
+
+    guard let first = first else { return [] }
+    return try dropFirst().reductions(first, transform)
+  }


In order to add this to Sequence without having to duplicate any logic, you could take the iterator, call next on it to get the seed, and then turn the iterator back into a sequence using IteratorSequence (which hopefully optimizes well).

danielctull · 2020-11-26T20:37:27Z

I'm writing the lazy version of reductions and hit a question with the empty case if you fine people could guide me. 🙂

Given that reductions now includes the initial seed value, does that mean should EmptyCollection<Int>().reductions(10, +) yield [10]? Or should it return an empty array?

odmir · 2020-11-27T01:46:43Z

The current implementation loses the "laziness" information requiring one to apply .lazy again to the result.

Thanks @odmir, the conditional conformances to LazySequenceProtocol and LazyCollectionProtocol was missing. Using your example now yields the following. 🙂
let a = [1, 2, 3, 4].lazy.prefix(2).map { $0 }
let b = [1, 2, 3, 4].lazy.reductions(0, +).map { $0 }

print(type(of: a))
// LazyMapSequence<Slice<LazySequence<Array<Int>>>, Int>
print(type(of: b))
// LazyMapSequence<Reductions<Int, LazySequence<Array<Int>>>, Int>

Of course that's the correct way of doing it, sorry for the noise! 🤦

xwu · 2020-11-27T02:01:39Z

Given that reductions now includes the initial seed value, does that mean should EmptyCollection<Int>().reductions(10, +) yield [10]?

Yes, and this appears to be how it behaves in other languages too. The version that takes an initial value would always have one more element in the result than are in the input collection, and the version that doesn’t, wouldn’t.

danielctull · 2020-11-27T07:35:19Z

Of course that's the correct way of doing it, sorry for the noise! 🤦

@odmir: No noise at all, I appreciate you raising the concern! 🙂

danielctull · 2020-11-27T07:57:57Z

Thanks for confirming that @xwu!

benlings · 2020-12-06T20:44:53Z

Would it be worth adding a deprecated alias scan for this (eg. like this? I have no problem with the name reductions, but would probably not have found it and assumed it didn't exist.

danielctull · 2021-01-29T17:05:06Z

Wow, I just remembered that I had this in flight. I moved house and have been really busy with that so haven't had much time to pursue this stuff.

Does anyone have any thoughts about @benlings' thought to include scan as a deprecated function to allow for easier discovery? Personally, I think the library is small enough that reductions would stand out on its own, but I'm happy to add a deprecated scan function.

jasdev · 2021-02-14T04:33:58Z

Does anyone have any thoughts about @benlings’ thought to include scan as a deprecated function to allow for easier discovery?

+1 from me — I actually stumbled upon this PR by searching around for “scan.”

Thanks for driving — and following up on — this PR, @danielctull! 🙏🏽

timvermeulen

It's nice to see this getting close to the finish line! I've left some more feedback. It would also be great to mark public methods with @inlinable and internal methods with @usableFromInline like we do in the rest of the package.

Let's indeed also add a deprecated scan method for discoverability purposes — we do not currently have a rigorous policy in place on when we want to add deprecated methods with names from prior art, but in this case scan is such a widely known name for this operation that it's worthwhile doing this.

Guides/Reductions.md

Sources/Algorithms/Reductions.swift

timvermeulen · 2021-02-17T21:27:02Z

Sources/Algorithms/Reductions.swift

+extension Sequence {
+
+  public func reductions<Result>(
+    _ initial: Result,
+    _ transform: (Result, Element) throws -> Result
+  ) rethrows -> [Result] {
+
+    var result = initial
+    return try reductions(into: &result) { result, element in
+      result = try transform(result, element)
+    }
+  }
+
+  public func reductions<Result>(
+    into initial: inout Result,
+    _ transform: (inout Result, Element) throws -> Void
+  ) rethrows -> [Result] {
+
+    var output = [Result]()
+    output.reserveCapacity(underestimatedCount + 1)
+    output.append(initial)
+
+    for element in self {
+      try transform(&initial, element)
+      output.append(initial)
+    }
+
+    return output
+  }
+}


These still need documentation, you can probably reiterate parts of the guide here. It's also a good idea to have a look at the reduce doc comments, since they're so related.

Sources/Algorithms/Reductions.swift

timvermeulen · 2021-02-17T21:41:24Z

Sources/Algorithms/Reductions.swift

+  enum Representation {
+    case start
+    case base(index: BaseIndex, result: Result)
+    case end
+  }


I've done some experiments and it doesn't seem like we need the start case at all, we can instead represent the start index by wrapping base.startIndex in the base case. Then you should be able to pretty much remove any code that currently uses .start — let me know if you run into any problems doing that.

timvermeulen · 2021-02-18T00:40:09Z

Sources/Algorithms/Reductions.swift

+extension LazySequenceProtocol {
+
+  public func reductions(
+    _ transform: @escaping (Element, Element) -> Element
+  ) -> InclusiveReductions<Self> {
+    InclusiveReductions(base: self, transform: transform)
+  }
+}
+
+extension Sequence {
+  public func reductions(
+    _ transform: (Element, Element) throws -> Element
+  ) rethrows -> [Element] {
+    var iterator = makeIterator()
+    guard let initial = iterator.next() else { return [] }
+    return try IteratorSequence(iterator).reductions(initial, transform)
+  }
+}


These are missing documentation as well. Might also be worth pointing out the relation to the version of reductions that takes an initial result.

danielctull · 2021-02-18T09:18:29Z

Cheers for those @timvermeulen! Will look at them over the next day. 😊

This will allow for future changes to either implementation without causing a breakage for the other.

timvermeulen · 2021-03-18T15:55:02Z

Hey @danielctull, this is to let you know we'll soon be landing this addition and making the minor changes necessary in order to ship this in the next release. 🚀 Thank you for all your work on this, and feel free to make any changes you still want to make in the meantime.

danielctull · 2021-03-18T16:05:50Z

Cheers for the prompt @timvermeulen, I actually have some time tonight to get the rest of the modifications and documentation changes made that you suggested if that's alright?

timvermeulen · 2021-03-18T16:23:05Z

Totally!

danielctull · 2021-03-19T16:07:03Z

The doc comments are there, but I know they need some re-working. The guide will also needs some tweaking to align with these revised comments. I'm planning to look at these tomorrow morning British time. 🙂

@timvermeulen: I'm personally not convinced about removing .start from the indexes because it seems to make it a little harder to reason about generally, happy to discuss this though.

…ductions

kylemacomber · 2021-03-23T18:17:54Z

@swift-ci please test

timvermeulen · 2021-03-24T15:55:56Z

@danielctull The last changes look great! The start case thing is a small tweak that we don't need to concern ourselves with right now, we can always revisit it later. Let's land this if you have no more changes you wanted to make.

danielctull · 2021-03-24T15:57:43Z

@timvermeulen: Thanks for that. Agreed, I'm all done with the changes for now. :)

danielctull marked this pull request as draft November 26, 2020 12:04

odmir reviewed Nov 26, 2020

View reviewed changes

Sources/Algorithms/Reductions.swift Outdated Show resolved Hide resolved

timvermeulen reviewed Nov 26, 2020

View reviewed changes

danielctull force-pushed the reductions branch 2 times, most recently from 7cbff5c to b8bd047 Compare December 3, 2020 08:03

timvermeulen suggested changes Feb 18, 2021

View reviewed changes

Use new lazy assertion functions

6b01737

danielctull force-pushed the reductions branch from b8bd047 to 6b01737 Compare February 25, 2021 08:27

danielctull added 4 commits February 25, 2021 08:32

Correct the complexity claims for reductions

714f46b

Separate the index types

5182363

This will allow for future changes to either implementation without causing a breakage for the other.

Update guide to reflect that arrays are returned directly

84b6ddc

Add scan as deprecated methods

fb004f7

danielctull added 10 commits March 18, 2021 16:47

Add lazy overload of reductions(into:_:)

7464876

Add the reductions(into:_:) functions

174efb8

More succinctly introduce reductions

1e47f86

Add a note about the deprecated scan functions

a73c681

Update documentation

f6b42ad

Add @inlinable and @usableFromInline

f7b90b5

Copy documentation to all variants of reductions

e9abcdb

Test the value after the function is complete

04f0a23

Just use the += operator

724e223

Add an example for reductions(into:_:)

042013f

danielctull added 3 commits March 21, 2021 12:35

Improve the documentation of the return value

5f55e8b

Update complexity note

d4daf2c

Use the reduce documentation for inspiration for the discussion of re…

6660d64

…ductions

danielctull marked this pull request as ready for review March 24, 2021 15:56

timvermeulen approved these changes Mar 24, 2021

View reviewed changes

timvermeulen merged commit b171b81 into apple:main Mar 24, 2021

danielctull deleted the reductions branch March 25, 2021 09:55

Add reductions(_:_:) #46

Add reductions(_:_:) #46

Conversation

danielctull commented Nov 24, 2020 • edited Loading

Description

Detailed Design

Documentation Plan

Test Plan

Source Impact

Checklist

CTMacUser commented Nov 25, 2020

danielctull commented Nov 25, 2020

timvermeulen commented Nov 25, 2020 • edited Loading

CTMacUser commented Nov 26, 2020

timvermeulen commented Nov 26, 2020

pyrtsa commented Nov 26, 2020

danielctull commented Nov 26, 2020 • edited Loading

pyrtsa commented Nov 26, 2020

timvermeulen commented Nov 26, 2020

danielctull commented Nov 26, 2020

pyrtsa commented Nov 26, 2020

timvermeulen commented Nov 26, 2020

CTMacUser commented Nov 26, 2020

odmir commented Nov 26, 2020

danielctull commented Nov 26, 2020 • edited Loading

timvermeulen Nov 26, 2020

Choose a reason for hiding this comment

danielctull commented Nov 26, 2020

odmir commented Nov 27, 2020

xwu commented Nov 27, 2020

danielctull commented Nov 27, 2020 • edited Loading

danielctull commented Nov 27, 2020

benlings commented Dec 6, 2020

danielctull commented Jan 29, 2021

jasdev commented Feb 14, 2021

timvermeulen left a comment

Choose a reason for hiding this comment

timvermeulen Feb 17, 2021

Choose a reason for hiding this comment

timvermeulen Feb 17, 2021

Choose a reason for hiding this comment

timvermeulen Feb 18, 2021

Choose a reason for hiding this comment

danielctull commented Feb 18, 2021

timvermeulen commented Mar 18, 2021

danielctull commented Mar 18, 2021

timvermeulen commented Mar 18, 2021

danielctull commented Mar 19, 2021 • edited Loading

kylemacomber commented Mar 23, 2021

timvermeulen commented Mar 24, 2021

danielctull commented Mar 24, 2021

danielctull commented Nov 24, 2020 •

edited

Loading

timvermeulen commented Nov 25, 2020 •

edited

Loading

danielctull commented Nov 26, 2020 •

edited

Loading

danielctull commented Nov 26, 2020 •

edited

Loading

danielctull commented Nov 27, 2020 •

edited

Loading

danielctull commented Mar 19, 2021 •

edited

Loading