Element Tutorial fixes

[jlstevens]

This PR addresses the issues raised in #516 regarding the Element tutorial, namely:

• Confusing bounds for Image.
• Incorrect names for x and y generated with np.mgrid

I've made the changes in such a way that I believe that most of the tests will continue to pass though Image will probably fail given the change of bounds.

[jlstevens]

More tests in Elements failed than I expected but it is not too surprising. The test data is now updating and I'll trigger a website build so we can check everything is alright.

[jbednar]

Thanks, but because the pattern is still symmetric, it's not clear whether the problem has been addressed. E.g. I think y is still flipped.

[philippjfr]

Yea, looks like y is still flipped, it's a bit ugly to have to flip them everywhere but I guess it's the right thing to do.

[jlstevens]

Do you think the sine rings shouldn't be symmetric? Maybe elongated along the x or y axis?

To be honest, I'm not sure how a user would notice the relationship between the x and y values and the output unless the x and y arrays are visualized as well. Unless you are very familiar, with mgrid you won't know which way the y values are flipped and even then it won't make a difference for a symmetric function (even if you elongate it).

[jbednar]

Right, that's the reason it's a very bad example; anyone who uses it as a starting point (which is what I was doing when I noticed it was wrong) will be scratching their head and thinking they were doing something wrong, when instead it's the example that's the problem. Thus, as an example (rather than just a sample), it's failing; we should be helping people (i.e., ourselves in this case :-), not misleading them. To avoid this problem we should use a function that has as few symmetries as is practical, and we've got to update the test data anyway.

Because we use these notebooks as tests, it would actually be good to use a rectangular shape rather than a square, to reveal any problems with switching coordinate order in our own code. At least this problem is only in the notebook! But that seems infeasible, since it would require extra code in each example, whereas just using a function that's not symmetric doesn't seem like it should make things significantly longer.

[jbednar]

E.g. here's one that's not symmetric:

That y=-y is very annoying, though!

[jbednar]

This one might be more obviously not symmetric across x:

[jlstevens]

I don't mind switching to these examples. What do you think @philippjfr?

[jbednar]

And I guess it should be y=np.flipud(y), not 'y=-y', which is even uglier.

[jbednar]

Here's one where the sizes are different, to make it clear that's supported and to show that y=-y wouldn't work:

It's a shame that mgrid doesn't seem to respect floating-point indexing, forcing the *0.1, or decreasing numbers, forcing the flipud. :-(

[jlstevens]

@jbednar Don't worry about using mgrid or flipud - my solution was to simply defined x and y once using meshgrid. That said, your latest example uses a different aspect so I would use something like:

x,y = np.meshgrid(np.linspace(-3,2,10), np.linspace(2,5,30))

[jbednar]

Makes sense; that's the more general and more easily understandable way to do it. I'm happy once it's updated to do that!

[jlstevens]

I've used the example you suggested though I added an offset as it was symmetric around zero in x:

np.sin(xs-1)+ys

This is quite a nice example! It is simple enough that I can reason about what the output should be in terms of the x and y inputs. Note that I named the variable xs and ys instead of x and y because these non-square arrays are used only in this example and I didn't want to clobber x and y arrays used later in the RGB example.

I'll rebuild the website though now we have to check for mgrid in the the other tutorials and won't we need to update the Bokeh elements tutorial now? :-(

[jbednar]

I thought the Bokeh tutorial was being built from the same source now? In any case, can you post the full proposed revised code using linspace/meshgrid? When I try it myself, y is still flipped.

[jlstevens]

Are you finding y flipped after d5efb78? That is what I added in my last commit and it seemed correct to me unless I am still confused somehow...

[philippjfr]

We have no system to build the bokeh tutorial from the same source and there are a still a few differences in plot and style options that would make that approach look pretty terrible.

[jbednar]

Ah, I see you reversed the order of the arguments to linspace to flip y. Ok, that works for me now; it's a little confusing because that's so hidden, but probably the best option.

However, I don't see why you need the -1, as sin(x) isn't symmetric across x=0. Maybe there's some symmetry I'm not seeing?

It looks symmetric across the center of the image, but it isn't quite. Maybe we should change the upper bound on X to make it not look symmetric?

Also, this is more speculative, but I wonder if it would be a clearer starting point if we made the bounds be specified in a single place in the code:

b=(-2,-3,4,2) # Coordinate system: (left, bottom, right, top)
xs,ys = np.meshgrid(np.linspace(b[0],b[2],50), np.linspace(b[3],b[1],30))
(hv.Image(np.sin(xs)+ys, bounds=b) +
 hv.Image(np.sin(xs)+ys, bounds=b)[0:3, -2.5:2])

[jbednar]

We have no system to build the bokeh tutorial from the same source and there are a still a few differences in plot and style options that would make that approach look pretty terrible.

Presumably those options are only specified in a few places? If so, seems like a simple sed script could patch them in. Or we could even show how to write notebooks that work well for multiple backends, by showing how to select different options for different backends based on detecting which backend is in use...

[jlstevens]

Ok this is a little hard to describe but I'll try!

In the first screenshot you posted, I look at the dark greyish wave under the red and you see that at x=0 you are halfway up the wave which is exactly what you expect for sin. The fact it almost looks symmetric in the image based on the chosen bounds is an accident: the bottom will be at pi/2 which happens to be near the middle of the image (x=1.5).

As sin is cyclic and the intensity as a function of x is passed through sin, I don't think you would notice a difference if the underlying values of x increased from right to left or left to right. The idea of the offset it moves the middle of the wave (again looking at the dark wave at the bottom) away from zero so it is clear that the underlying x values increase from left to right.

I hope that makes sense! My argument here doesn't relates to the bounds at all so maybe there is another issue to address that I don't quite understand yet.

[jbednar]

I'm still confused; sin is cyclic, but not symmetric across the y axis. I.e., sin(x) != sin(-x), in general. That's also true of sin(x-1), but no more than of sin(x) itself. E.g. at y=0:

Also note that in my code here I've fixed the comment about "Coordinate system"; it's lbrt, not lbtr as it has been claiming.

[jlstevens]

I'm still confused; sin is cyclic, but not symmetric across the x axis. I.e., sin(x) != sin(-x), in general.

True. I just thought an offset might be more obvious but yes, you are right that there is enough information (you just need to look at the shape around x=0 and think a little bit about it). I'll go ahead and remove the offset value...

Would you still like to change the bounds?

[jbednar]

Thanks. Yes, I think having the different bounds helps make it more obvious what matches up to what.

[philippjfr]

Is this almost ready to merge? Are we ready to announce?

[jlstevens]

I've just made the last suggested change. If you are happy with it now, I'll update the test data and it can then be merged. What about updating the other tutorials where mgrid was used?

[jbednar]

But according to the diffs, mgrid is still in use even in this tutorial, and therefore is silently flipping y?

[jlstevens]

Those remaining uses of mgrid are in VectorField and Surface3D where the issue of ordering doesn't come up (they have explicit samples values at every point). I've double checked and it looks correct to me after those two uses of mgrid!

[jbednar]

Ok, sounds good! Please merge and when the web site is ready I'll announce.

[philippjfr]

What about updating the other tutorials where mgrid was used?

Presumably they should be updated as part of this PR as well, or is there a reason not to do that?

[jbednar]

Yes, any tutorial where mgrid is used is potentially a source of confusion, unless it eventually ends up as specific coords as Jean-Luc points out above. I don't know if I'm typical, but I tend to go to Elements for examples to use as starting points, so that's the one I most care about not being misleading, and the others seem lower in priority (i.e., not worth holding up the announcement). But if they can be fixed quickly, it would be good to get it over with in this one PR.

[jlstevens]

Presumably they should be updated as part of this PR as well, or is there a reason not to do that?

I would prefer to update them as separate PRs if only because it is easier to catch problems with the tests that way. If we have too many test changes, it is hard to spot any transient errors that we don't want to slip onto master.

[jbednar]

Ok, sounds good to me.

[jlstevens]

The test data is updating. Please merge once the PR build goes green.

[philippjfr]

I would prefer to update them as separate PRs if only because it is easier to catch problems with the tests that way.

Sure, I don't mind but personally I'd check them thoroughly once, rather than waiting on it to rebuild it multiple times with a decent chance of a transient being introduced each time.

Will merge once tests are passing.

[jbednar]

Looks like a test failed, but I'm not sure what?

[philippjfr]

Failed a bunch of times because a transient error was captured, seems to be working now. The push build failing is expected since master has changed. Happy to merge now, unless you want to merge in master first.

[jlstevens]

@philippjfr If you are happy there are no transients (shouldn't be now the pr build is passing) please go ahead and merge.

[jbednar]

I don't see the updated Image example on build.holoviews.org, dev.holoviews.org, or holoviews.org, but let me know once it should be done propagating to the public site and I'll announce.

[github-actions]

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