Revert "Fix broadcast_indices (#22130)" & add test for issue

[vtjnash]

This reverts commit fb81c34 from #22130.
And adds a test for the bug this causes.

[nalimilan]

Can you split the example into two parts rather than changing it back? It's useful to test implementations that override setindex!.

[nalimilan]

So at least now I understand why you changed this.

Still, telling people to override _broadcast_indices doesn't feel right. If even functions starting with an underscore are not private, I don't know which functions would. Maybe we should swap broadcast_indices and _broadcast_indices so that packages override the former and internal code calls the latter? Or we could use completely different names. It would also be nice to avoid breaking packages which implement custom broadcast methods.

[andreasnoack]

It would also be nice to avoid breaking packages which implement custom broadcast methods.

I think that would be okay if we in return get a more extensible design for broadcast. The current version is really not well fit for extensions where _broadcast_indices is just a minor example.

[nalimilan]

I think that would be okay if we in return get a more extensible design for broadcast. The current version is really not well fit for extensions where _broadcast_indices is just a minor example.

Yes, I don't mind breaking things if that allows defining a better API that will remain stable after that. But here we just rename functions without a clear global picture for 1.0 AFAICT (see #20740).

[vtjnash]

Still, telling people to override _broadcast_indices doesn't feel right

Yeah, I didn't really investigate that. But as counterpoint, you only should be overriding this method if you also override _containertype, so this is more locally consistent.

It would also be nice to avoid breaking packages which implement custom broadcast methods.

It's already broken just trying to use broadcast without packages, as demonstrated by the test added herein. I don't have any perspective on the global picture for this API in the future, I just know that this is a simple fix for a case that is broken now. We can review and hope for a more elegant solution in the future, but I don't agree that should hold up a simple-enough bug fix in the present design.

[nalimilan]

Yeah, I didn't really investigate that. But as counterpoint, you only should be overriding this method if you also override _containertype, so this is more locally consistent.

Except that it looks like we'd better rename _containertype before 1.0. Let's not align all functions to the worse example in the broadcast API...

It's already broken just trying to use broadcast without packages, as demonstrated by the test added herein. I don't have any perspective on the global picture for this API in the future, I just know that this is a simple fix for a case that is broken now. We can review and hope for a more elegant solution in the future, but I don't agree that should hold up a simple-enough bug fix in the present design.

Well, broadcast was broken in one use case, and you break it for all use cases until packages have been updated. So we shouldn't merge it if we know we'll have to make another change in a few weeks. Also this isn't backportable to 0.6 since it will break packages: either we need to find a less intrusive fix, or we won't backport it and there's no hurry to fix it in 0.7 either.

Isn't there a way to fix this while still keeping broadcast_indices?

[vtjnash]

Isn't there a way to fix this while still keeping broadcast_indices?

No, you can't give two different functions the same name

[TotalVerb]

I think we should swap the two function names... This can't be deprecated and however would simply break code.

[vtjnash]

That still breaks DataArrays (packages that define _broadcast_indices also need to call broadcast_indices), but seems like it would be less consistent within the current API.

[nalimilan]

Then can we just hold off merging this for a while and try to figure what the final design will look like in the meantime? E.g. we might want to rename _broadcast_indices to broadcast_indices_rule, and _promote_containertype to promote_containertype_rule for consistency with promote and promote_rule (and to avoid the need to have two definitions for each pair of types). Maybe _containertype and containertype could be swapped, since it doesn't seem packages need to call the latter, which is just a small convenience function which takes instances rather than types.

[ararslan]

Adding the decision label since a decision is needed on the design. Should we split the design discussion into a separate issue and/or Julep?

[Sacha0]

Ref. #20740

[JeffBezanson]

Bump. We need to figure out how to move forward here.

[nalimilan]

Comments on the proposal I made in my last comment ?

[ararslan]

That proposal sounds reasonable to me

[timholy]

Redundant with #23939

[vtjnash]

Yay!