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

Support for cycle #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support for cycle #17

wants to merge 1 commit into from

Conversation

camspiers
Copy link
Contributor

Takes an iterator and cycles through its elements a given number of times

If working with a generator it is best to make it rewindable if the expected
number of elements is large

Examples:

  iter\cycle([1, 2, 3], 3)
  => iter(1, 2, 3, 1, 2, 3, 1, 2, 3)

 Takes an iterator and cycles through its elements a given number of times

 If working with a generator it is best to make it rewindable if the expected
 number of elements is large

 Examples:

      iter\cycle([1, 2, 3], 3)
      => iter(1, 2, 3, 1, 2, 3, 1, 2, 3)
*
* @return \Iterator
*/
function cycle($iterable, $num = INF) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this run endless in case $num is not provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it will :) it is designed as repeat is and how the python cycle function is designed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staabm It is useful because if you create conditions that stop iterating the generator then it wont run forever. In other words you get a chance to break the iteration after each yield.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't there be a break or something like that after all values have been yielded?ä
How would the caller of this function otherwise break the endless loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It cycles through the values of the iterator yielding each value until $num cycles are complete. If $num is INF then the generator will continue yielding values from the iterator until iteration of the generator stops (which may be never).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a non-useful but informative example:

// this will run forever
foreach (cycle(['A', 'B']) as $value) {

}
// this will run until the condition is satisfied
foreach (cycle(['A', 'B']) as $value) {
    if (mt_rand(1, 100) === 50) {
       break;
   }
}

function cycle($iterable, $num = INF) {
if ($iterable instanceof \Generator) {
$iterable = toArray($iterable);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing I'm unsure about here is this automatic toArray conversion. The iterator below preserves keys, so this should probably be toArrayWithKeys -- however, converting generators with keys will very often not produce the desired result due to overwritten keys. One could store keys and values in separate arrays to avoid that, though that'll make the implementation quite ugly.

@vkurdin
Copy link

vkurdin commented Jan 25, 2018

@camspiers @nikic maybe something like this?

function cycle(iterable $iterable): \Iterator {
    $isIterator = $iterable instanceof \Iterator;
    
    do {
        yield from $iterable;

        if ($isIterator) {
            $iterable->rewind();
        }
    } while (!$isIterator || $iterable->valid());
}

@vkurdin
Copy link

vkurdin commented Jan 25, 2018

with counter:

function cycle(iterable $iterable, $num = INF): \Iterator{
    $isIterator = $iterable instanceof \Iterator;

    for ($i = 0; ($i < $num) && (!$isIterator || $iterable->valid()); ++$i) {
        yield from $iterable;

        if ($isIterator) {
            $iterable->rewind();
        }
    }
}

flatMap variant:

function cycle(iterable $iterable, $num = INF): \Iterator {
    $isIterator = $iterable instanceof \Iterator;

    return flatMap(
        function ($iterable) use ($isIterator) {
            if ($isIterator && !$iterable->valid()) {
                $iterable->rewind();
            }

            return $iterable;
        },
        repeat($iterable, $num)
    );
}

@nikic
Copy link
Owner

nikic commented Jan 25, 2018

@vkurdin The issue here is mainly that iterators are not always rewindable. In particular generators (which is what this library uses) are never rewindable.

@vkurdin
Copy link

vkurdin commented Jan 25, 2018

@nikic how's this one? Ugly enough? :)

function cycle(iterable $iterable, $num = INF): \Iterator {
    if ($iterable instanceof \Generator) {
        $pairs = [];
        $iterableInner = fromPairs(
            map(
                function ($pair) use (&$pairs) {
                    return $pairs[] = $pair;
                },
                toPairs($iterable)
            )
        );
    } else {
        $iterableInner = $iterable;
    }

    for ($i = 0; $i < $num; ++$i) {
        yield from $iterableInner;

        if ($iterable instanceof \Generator) {
            $iterableInner = fromPairs($pairs);
        } elseif ($iterable instanceof \Iterator) {
            $iterable->rewind();
        }
    }
}

@ragboyjr
Copy link
Contributor

This works as well:

function cycle($iterable, $num = INF) {
    if (is_array($iterable)) {
        for ($i = 0; $i < $num; $i++) {
            yield from $iterable;
        }
        return;
    }

    // if iterable, we need to store as array first
    $pairs = toArray(toPairs($iterable));
    for ($i = 0; $i < $num; $i++) {
        yield from fromPairs($pairs);
    }
}

it optimizes for the case of array, but not necessarily needed. Here are some tests which work that showcase:

    public function testCycle() {
        $this->assertSame([1,2,1,2], toArray(cycle([1, 2], 2)));
    }
    public function testCycleIter() {
        $gen = function() {
            yield 'a' => 1; yield 'a' => 2;
        };
        $this->assertSame(
            [
                ['a', 1],
                ['a', 2],
                ['a', 1],
                ['a', 2],
            ],
            toArray(toPairs(cycle($gen(), 2)))
        );
    }

@vkurdin
Copy link

vkurdin commented Jan 25, 2018

@nikic generators are never rewindable, but can be passed as iterable to many \iter functions with implicit rewind inside foreach loop

@vkurdin
Copy link

vkurdin commented Jan 25, 2018

@ragboyjr in your example keys from iterable are lost after toArray call, mentioned before: #17 (comment)

@ragboyjr
Copy link
Contributor

@vkurdin the to and from Pairs takes care of that. the testCycleIter shows that the keys aren't being lost/overwitten as well.

@vkurdin
Copy link

vkurdin commented Jan 25, 2018

@ragboyjr sorry, did not noticed 😔

@vkurdin
Copy link

vkurdin commented Jan 25, 2018

@ragboyjr toArray also causes allocation of all values from generator at the beginning with lost of generator-like laziness

@ragboyjr
Copy link
Contributor

Y, there's no way around that. Some iters are rewindable, some aren't, but it's impossible to distinguish from the two. So the only way this implementation is to work is that the cycled contents are stored/cached.

The only other option is that we just assume it's rewindable and let it throw an exception if not. There is the iter\makeRewindable function which does that caching. So instead of always caching the iterators, we put that on the user.

@drupol
Copy link

drupol commented Sep 26, 2020

I implemented Cycle using InfiniteIterator, see: https://github.com/loophp/collection/blob/master/src/Operation/Cycle.php

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.

6 participants