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

Implement zip_broadcast #545

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Implement zip_broadcast #545

merged 3 commits into from
Aug 31, 2021

Conversation

bbayles
Copy link
Collaborator

@bbayles bbayles commented Aug 30, 2021

Re: #543, this PR adds zip_broadcast. See the issue for discussion.

@bbayles
Copy link
Collaborator Author

bbayles commented Aug 30, 2021

Tests for 3.10 are failing due to python/typing#865.

@kalekundert
Copy link
Contributor

Thanks for implementing this; it does do what I want. I have a few comments, though:

  1. The function returns the wrong value when *objects is empty. It yields a single empty tuple, when it should yield nothing.
  2. I think zip_fill() is a better name than zip_broadcast(). The reason is that the ability to support 2- and 3-dimensional inputs is a big part of what np.broadcast() is used for, while this is function is specifically for 1-dimensional inputs.
  3. Both this function and always_iterable() have the same test to see if an object is iterable. Would it make sense to factor this into it's own publicly-available function?

Here are the tests I wrote for my implementation. I have some test cases that you don't, so you might want to include them:

@pytest.mark.parametrize(
        "objs, kwargs, expected", [(
            # 0 items
            [],
            {},
            [],
        ), (
            [[]],
            {},
            [],
        ), (
            # 1 item
            [1],
            {},
            [(1,)],
        ), (
            [[1]],
            {},
            [(1,)],
        ), (
            [[1], 2],
            {},
            [(1, 2)],
        ), (
            [1, [2]],
            {},
            [(1, 2)],
        ), (
            [[1], [2]],
            {},
            [(1, 2)],
        ), (
            # 2 items
            [[1, 2]],
            {},
            [(1,), (2,)],
        ), (
            [[1, 2], 3],
            {},
            [(1, 3), (2, 3)],
        ), (
            [1, [2, 3]],
            {},
            [(1, 2), (1, 3)],
        ), (
            [[1, 2], [3, 4]],
            {},
            [(1, 3), (2, 4)],
        ), (
            # not iterable
            ['ab'],
            {},
            [('ab',)],
        ), (
            ['ab'],
            dict(not_iterable=None),
            [('a',), ('b',)],
        ), (
            [[1, 2]],
            dict(not_iterable=list),
            [([1, 2],)],
        ), (
            # unequal length
            [[], [1]],
            {},
            [],
        ), (
            [[1], [2, 3]],
            {},
            [(1, 2)],
        ), (
            # infinite
            [1, repeat(2), [3]],
            {},
            [(1, 2, 3)],
        )],
)
def test_zip_with_scalars(objs, kwargs, expected):
    assert list(zip_with_scalars(*objs, **kwargs)) == expected

@pytest.mark.parametrize(
        "objs, kwargs, error", [(
            # 0 items
            [[], [1]],
            dict(strict=True),
            UnequalIterablesError,
        ), (
            [[1], [2, 3]],
            dict(strict=True),
            UnequalIterablesError,
        )],
)
def test_zip_with_scalars_err(objs, kwargs, error):
    with pytest.raises(error):
        list(zip_with_scalars(*objs, **kwargs))

@kalekundert
Copy link
Contributor

zip_repeat() might be another reasonable name.

@bbayles
Copy link
Collaborator Author

bbayles commented Aug 31, 2021

Thanks for the review. I'm going to go with the zip_broadcast name. I considered doing an _is_iterable function, but I'll wait for a third consumre of it. Your point about the empty input was right on.

@bbayles bbayles merged commit 004e0df into master Aug 31, 2021
@bbayles bbayles deleted the zip-broadcast branch August 31, 2021 13:39
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