Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚧 Add simpler Unfold overload #1015

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

🚧 Add simpler Unfold overload #1015

wants to merge 8 commits into from

Conversation

atifaziz
Copy link
Member

@atifaziz atifaziz commented Oct 14, 2023

This PR adds a simpler overload of Unfold that requires only a single function instead of 3. Example usage:

var xs = MoreEnumerable.Unfold(1, s => s <= 100 ? (true, s * 2, s) : default);
Console.WriteLine(string.Join(", ", xs));

Prints:

1, 2, 4, 8, 16, 32, 64

Pending work:

  • API design: (bool, TState, TResult) or (bool, (TState, TResult))?
  • API design: TState then TResult or TResult then TState?
  • Add unit tests

@viceroypenguin
Copy link
Contributor

How is this different than MoreEnumerable.Generate(1, s => s *2).TakeWhile(s => s <= 100)?

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Merging #1015 (1559a27) into master (5eebc0a) will increase coverage by 0.00%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 1559a27 differs from pull request most recent head 4fcd56f. Consider uploading reports for the commit 4fcd56f to get more accurate results

@@           Coverage Diff           @@
##           master    #1015   +/-   ##
=======================================
  Coverage   92.57%   92.58%           
=======================================
  Files         113      113           
  Lines        3422     3426    +4     
  Branches     1055     1057    +2     
=======================================
+ Hits         3168     3172    +4     
  Misses        191      191           
  Partials       63       63           
Files Coverage Δ
MoreLinq/Batch.cs 98.07% <ø> (ø)
MoreLinq/Unfold.cs 95.00% <100.00%> (+1.25%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@atifaziz
Copy link
Member Author

atifaziz commented Oct 15, 2023

How is this different than MoreEnumerable.Generate(1, s => s *2).TakeWhile(s => s <= 100)?

That specific example is not, but Unfold anyway is Generate + TakeWhile + Select rolled into one. If TState and TResult are identical then Select can be dropped. Historically, Generate was there first, back in 2009. Unfold was added later in 2017. Either can be written in terms of the other.

For comparison, below is a slightly more involved example that generates the Fibonacci sequence where all numbers are less than 100. Using Generate + TakeWhile + Select (requires 3 lambdas):

var fibonacciNumbersLowerThan100 =
    MoreEnumerable.Generate((Curr: 0, Next: 1), s => (s.Next, s.Curr + s.Next))
                  .TakeWhile(s => s.Curr < 100)
                  .Select(s => s.Curr);

Using Unfold (requires 4 lambdas):

var fibonacciNumbersLowerThan100 =
    MoreEnumerable.Unfold((Curr: 0, Next: 1),
                          s => (s.Curr, Next: (Curr: s.Next, Next: s.Curr + s.Next)),
                          s => s.Curr < 100,
                          s => s.Next,
                          s => s.Curr);

Using the overload proposed here (requires 1 lambda):

var fibonacciNumbersLowerThan100 =
    MoreEnumerable.Unfold((Curr: 0, Next: 1),
                          s => s.Curr < 100 ? (true, (s.Next, s.Curr + s.Next), s.Curr) : default);

@viceroypenguin
Copy link
Contributor

nit: for this example, this would be cleaner as it does not rely on a ternary or default

var fibonacciNumbersLowerThan100 =
    MoreEnumerable.Unfold((Curr: 0, Next: 1),
                          s => (s.Curr < 100, (s.Next, s.Curr + s.Next), s.Curr));

That said, I'm curious to whom you see this as a potential benefit.

  • I don't see this version of Unfold (or even the original one) as simpler than Generate().TakeWhile().Select(); on the contrary, I'd argue that Generate() is more advantageous because it relies on composability which is both more flexible and more consistent in reading.
  • I'm not sure that I see this version of Unfold as using less lambdas, because the implementation requires three lambdas, so it is still a four-lambda solution in the end.
  • I might be able to buy a performance argument, but I'd have to see a benchmark to confirm the validity.

@atifaziz
Copy link
Member Author

nit: for this example, this would be cleaner as it does not rely on a ternary or default

Yes, but with the ternary (for illustration) and more generally, you avoid wasteful computation (like even s.Curr + s.Next) if not needed because the break condition's been met.

That said, I'm curious to whom you see this as a potential benefit.

People used to unfold from other languages and/or runtimes/libraries (such as someone used to Seq.unfold in F#) will find it more aligned and natural.

  • I don't see this version of Unfold (or even the original one) as simpler than Generate().TakeWhile().Select(); on the contrary, I'd argue that Generate() is more advantageous because it relies on composability which is both more flexible and more consistent in reading.

It's not trying to be simpler than Generate + TakeWhile + Select. It's overloading the existing Unfold with a simpler version.

  • I'm not sure that I see this version of Unfold as using less lambdas, because the implementation requires three lambdas, so it is still a four-lambda solution in the end.

The implementation is immaterial as it can change so the point I was makign is how many lambdas the developer has to author.

  • I might be able to buy a performance argument, but I'd have to see a benchmark to confirm the validity.

There's no performance argument being made for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants