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 9 commits into
base: master
Choose a base branch
from
Draft

🚧 Add simpler Unfold overload #1015

wants to merge 9 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

All modified and coverable lines are covered by tests ✅

Project coverage is 93.24%. Comparing base (5dc9e18) to head (005f7d4).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   93.23%   93.24%   +0.01%     
==========================================
  Files         112      112              
  Lines        3400     3407       +7     
  Branches      962      965       +3     
==========================================
+ Hits         3170     3177       +7     
  Misses        214      214              
  Partials       16       16              
Flag Coverage Δ
93.24% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@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

atifaziz commented Oct 17, 2023

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 making 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.

2 participants