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 overlay on Layouts #2596

Closed
jbednar opened this issue Apr 21, 2018 · 15 comments
Closed

Support overlay on Layouts #2596

jbednar opened this issue Apr 21, 2018 · 15 comments
Labels
type: enhancement Minor feature or improvement to an existing feature
Milestone

Comments

@jbednar
Copy link
Member

jbednar commented Apr 21, 2018

Is there a deep reason why overlaying an annotation onto a Layout doesn't work?

image

Of course, I can overlay it on each of the items in the layout, but can't the overlay operation simply iterate through the items in the layout and overlay the annotation on each one? That's what happens for NdLayouts and GridSpaces, so I'm not sure why it doesn't work here. Sure, there can be times that doing so will lead to an error, but I don't think those times should stop it working when it makes sense as it does here.

@jlstevens
Copy link
Contributor

This might be ok in cases like this where you are overlaying a simple element but we are more uncertain of how to distribute containers, e.g layout * layout for example...

Generally we never intended to support * at the layout level.

@jbednar
Copy link
Member Author

jbednar commented Apr 22, 2018

I'm pretty sure that Layout*Annotation always makes sense and that it's very useful, because annotating the same location on a bunch of plots is a very reasonable thing to want to do. Layout*Element in general also seems like it it would make sense, i.e. to see how one set of data relates to a lot of other sets of data. Both of these are a distributive operation of scalars over lists, which is very clearly meaningful and useful.

Layout*Layout would presumably be elementwise multiplication (not cross product! :-), which is well defined when the lists are the same length, and seems like it would be very useful in some cases, but maybe those don't come up often. If they are different lengths, I'm sure we could define a default behavior, but my gut feeling is that such a default is very unlikely to be very useful, so it seems reasonable not to allow it unless some amazing application for it comes up.

So I vote for L*E to be supported, and L*L to be supported when the lists are the same length.

@philippjfr
Copy link
Member

Seems fine. I'd be happy to support Layout * Element/Overlay/HoloMap/DynamicMap but nothing else.

@jlstevens
Copy link
Contributor

jlstevens commented Apr 23, 2018

We have discussed the elementwise zipping semantics for layout * layout a number of times but we have never really known how to feel about it. Supporting the components Philipp listed makes sense at this time though.

@jbednar
Copy link
Member Author

jbednar commented Apr 23, 2018

Is there an issue documenting the L*L discussion that I could look at? To me that seems very well defined (when they are the same length) and sometimes useful, and supporting it seems reasonable to me, but I may not have thought of some subtlety. I'm not arguing strongly about it, just don't see the problem.

@philippjfr
Copy link
Member

philippjfr commented Apr 23, 2018

(when they are the same length)

That doesn't sound very well defined to me, what happens if they are not the same length? Supporting just the very specific condition when they match in length seems weird to me. Some of that discussion is here: #2075 but some of it wasn't captured I'm guessing.

I'm fairly strongly against the zipping suggestion since it's a pretty unpredictable thing to do.

@jbednar
Copy link
Member Author

jbednar commented Apr 23, 2018

I'm not seeing what the unpredictability is here, if it raises an exception when they are not the same length. Seems very straightforward to me. Again, I don't know how useful it is, but it does seem straightforward.

@jlstevens
Copy link
Contributor

This is what I mean about us not knowing what we feel about it!

That is why we should tackle the case we do agree makes sense first allowing us to decide how to handle layout * layout later.

@philippjfr
Copy link
Member

philippjfr commented Apr 23, 2018

Seems very straightforward to me.

Sure it's straightforward in itself but it's also hard to discover and raises all kinds of expectations about what might and might not be supported. A user who tries it and gets an exception will assume it's not supported while a user who tries it and it does work will start expecting it to always work. For all the other types the semantics of * are very straightforward and unambiguous and therefore don't have the need for restrictions like "if they are the same length". The mere fact that we are having this discussion shows that's not the case here. That definitely makes me question that the utility of allowing it in one very specific case outweighs the possible confusion it might cause about what is and isn't supported.

@jbednar
Copy link
Member Author

jbednar commented Apr 23, 2018

I guess I disagree with that approach in itself, then. I think that if we support the cases that are straightforward, then that will cover what normal people will do, and they don't need to even ever dream that there are non-straightforward cases. Whereas if we fail to allow what is very clearly meaningful, many people will try it and have no idea why it doesn't work, then move on slightly bewildered. Basically, people really don't care what happens for some case they aren't trying, so if it works for the cases they do try, then they are happy. Why should the fact that some cases are not straightforward have any bearing on the ones that are?

@jbednar
Copy link
Member Author

jbednar commented Apr 23, 2018

The mere fact that we are having this discussion is a case in point -- I expected L*L to work for equal-length lists, and I know exactly what should happen in that case, and I think everyone will agree on what they think should happen there if it's supported. So I would have been happy and moved on with my life, never worrying about what would happen if the lengths were unequal, since I'm not going to try that (and if I did, I would have no idea what to expect, and would not be the least bit surprised for that to raise an exception).

@philippjfr philippjfr added the type: enhancement Minor feature or improvement to an existing feature label Apr 25, 2018
@philippjfr philippjfr added this to the v1.11 milestone Apr 25, 2018
@philippjfr
Copy link
Member

philippjfr commented Dec 1, 2018

I'm still at a strong -1 for allowing overlays of two Layouts, the zipping semantics is only one possibility you could imagine, while all other supported overlay operations are all very clearly defined. Indeed those semantics differ substantially to the behavior of NdLayouts which are matched on their keys:

screen shot 2018-12-01 at 1 33 22 pm

There is no other overlay operation in holoviews that has zipping semantics so this is an entirely new concept to introduce. In all my years of using holoviews I have also never once thought to try doing this and to be quite honest overlaying by position rather than by matching on some key also scares me too much to ever consider using it. It's simply way too easy to get the ordering wrong and end up with something you didn't intend.

I think that if we support the cases that are straightforward, then that will cover what normal people will do, and they don't need to even ever dream that there are non-straightforward cases.

My objection isn't about it being straightforward or not, indeed a lot of the overlay semantics are by far more complex and less straightforward, my objection is that it does not match other semantics and that it's much more implicit than anything else. When you can match on keys like NdLayouts or GridSpaces do, you can be sure you are doing the semantically "correct" thing. When you zip two lists you are using implicit information, i.e. the length and ordering of lists, to do that matching which has no guarantee at all of being correct and lots of potential to be incorrect, particularly when the lists are long and you can't easily visually confirm that the two lists are in the order you intend.

I intend to merge my PR to support Element/Overlay/HoloMap/DynamicMap overlays and then close this issue.

@jbednar
Copy link
Member Author

jbednar commented Dec 1, 2018

I have no strong commitment to L*L other than to have expected it to work, so it's fine to decide not to support it. But this issue isn't about L*L, which only came up later, it's about L*A (broadcasting, not zipping).

ETA: Oh, now that your PR appears I see that it's about overlays onto layouts, i.e., that you are now supporting broadcasting, just not zipping. In that case I agree, this issue can be closed when #3234 is merged. Thanks!

@philippjfr
Copy link
Member

But this issue isn't about LL, which only came up later, it's about LA (broadcasting, not zipping).

Which I've implemented in the PR referenced above.

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants