penguin icon indicating copy to clipboard operation
penguin copied to clipboard

Refactor co-primes algorithm to be generic & avoid allocation.

Open saeta opened this issue 5 years ago • 7 comments

Thank you @dabrahams for the suggestions!

saeta avatar May 23 '20 23:05 saeta

This is a follow-up to https://github.com/saeta/penguin/pull/30

saeta avatar May 23 '20 23:05 saeta

This relates to umbrella issue: https://github.com/saeta/penguin/issues/33

saeta avatar May 23 '20 23:05 saeta

I wondered whether computing GCD for every value < N was the best way and landed on this: https://math.stackexchange.com/questions/621109/co-prime-numbers-less-than-n

I didn't analyze it, but something to think about… someday. ;-)

dabrahams avatar May 24 '20 21:05 dabrahams

Switched from Sequence to Collection. It's an interesting point in design space. As alluded to, I'm somewhat skeptical that we're getting appropriately efficient operations by default. (e.g. Sequence.prefix avoids redundant computations (at the cost of memory consumption), whereas (as written for now), Collection.prefix doesn't. Probably something to refine further over time, such as defining a SubSequence instead of accepting the Collection's default, but not sure if that's enough; haven't thought enough about it yet.)

I'm sure some of the doc comments could be improved... feedback welcome!

saeta avatar Jun 07 '20 09:06 saeta

@dabrahams aside from our discussion on Sequence vs Collection, do you have further changes you'd like to see?

saeta avatar Jun 09 '20 19:06 saeta

Whoops, looks like your additional comments raced with me fixing & pushing the fixes! PTAL?

saeta avatar Jun 19 '20 20:06 saeta

Of course, we could make this be a LazyPrefixWhileSequence<SubSequence>, but this forces closure allocation.

Forces allocation? Only if there's a capture and only if you store the thing somewhere such that the closure can't be inlined.

dabrahams avatar Jun 22 '20 16:06 dabrahams