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

Make broadcasting over sets an error #28491

Closed
wants to merge 1 commit into from
Closed

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Aug 7, 2018

Ref #23746.

@ararslan ararslan mentioned this pull request Aug 7, 2018
7 tasks
@JeffBezanson JeffBezanson added this to the 1.0 milestone Aug 7, 2018
@vtjnash
Copy link
Member

vtjnash commented Aug 7, 2018

I don't think we should be adding random special cases here. For one thing, there exist OrderedSets – and we're even considering specifying them as the default in base (#10116), when we get some time to finish testing – so I disagree that it should require a special case here banning all AbstractSets.

Even more importantly, even though the ordering is undefined, it is guaranteed to be stable, so there can be cases where it would make sense to use broadcast where one of the iterable arguments happens to be a Set. An arbitrary artificial example is:

make_row(k, v, av) -> (pk=k, value=v, absvalue=av)
dictionary_to_table(dictionary) = make_row.(keys(dictionary), values(dictionary), abs.(values(dictionary)))

@mbauman
Copy link
Member Author

mbauman commented Aug 7, 2018

Ah, phew — good! Also, I found #26980 this morning, which I had missed last night. The plan I enumerated there is exactly the status quo on master right now.

I just wanted to be as conservative in the deprecation removal process as is reasonable.

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.

3 participants