Fixed bug unpacking Overlay and Layout objects in Layout.from_values

[philippjfr]

Fix for #1081, passing Layouts and Overlays to their respective constructors correctly unpacks them again after regression caused them to be nested.

elements = [hv.Curve(np.random.rand(10)) * hv.HLine(0) for i in range(4)]
overlay = hv.Overlay(elements)
print(repr(overlay))

This now correctly produces an object with this repr:

:Overlay
   .Curve.I   :Curve   [x]   (y)
   .HLine.I   :HLine   [x,y]
   .Curve.II  :Curve   [x]   (y)
   .HLine.III :HLine   [x,y]
   .Curve.III :Curve   [x]   (y)
   .HLine.IV  :HLine   [x,y]
   .Curve.IV  :Curve   [x]   (y)
   .HLine.V   :HLine   [x,y]

[philippjfr]

Just noticed that it's skipping HLine.II instead adding HLine.V this was also the case before the regression but it seems like a bug.

[philippjfr]

With my latest fix the repr now looks correct.

:Overlay
   .Curve.I   :Curve   [x]   (y)
   .HLine.I   :HLine   [x,y]
   .Curve.II  :Curve   [x]   (y)
   .HLine.II  :HLine   [x,y]
   .Curve.III :Curve   [x]   (y)
   .HLine.III :HLine   [x,y]
   .Curve.IV  :Curve   [x]   (y)
   .HLine.IV  :HLine   [x,y]

Going to need a lot of unit tests.

[jlstevens]

Looks good but I will wait to see examples of what the _unpack_paths classmethod does in the unit tests before carrying out a more detailed review.

[philippjfr]

There's definitely still things going wrong. Will have to look into this more.

[philippjfr]

Finally managed to resolve this, still some unit tests to add but the current notebook test failure in Showcase is real:

freq1 = hv.Image(sine(grid, freq=50))  + hv.Curve(zip(dist, sine(dist**2, freq=50)))
freq2 = hv.Image(sine(grid, freq=200)) + hv.Curve(zip(dist, sine(dist**2, freq=200)))
(freq1 + freq2).cols(2)

With the PR

:Layout
   .Image.I  :Image   [x,y]   (z)
   .Curve.I  :Curve   [x]   (y)
   .Image.II :Image   [x,y]   (z)
   .Curve.II :Curve   [x]   (y)

Without the PR:

:Layout
   .Image.I   :Image   [x,y]   (z)
   .Curve.I   :Curve   [x]   (y)
   .Image.II  :Image   [x,y]   (z)
   .Curve.III :Curve   [x]   (y)

Hopefully this is the only thing that snuck in while our tests weren't running properly.

[jlstevens]

I agree the new repr looks correct. Happy to merge once the tests are updated and passing.

[jlstevens]

It would still be nice to have examples of what the _unpack_paths classmethod does in unit tests. Otherwise, I am happy to merge once the notebook tests are passing.

[philippjfr]

So I just checked out some old versions and it turns out the issue above, which is now been fixed, has always been there.

[philippjfr]

So this code turned out to be quite a mess, the functionality was duplicated across two methods and spread out even further into various __mul__ and __add__ methods. The new approach is a) more correct (as evidenced by the bug mentioned above) and b) more consistent as paths are always recomputed in the same way rather than relying on brittle code to rename existing paths.

The new algorithm can be summed up in two steps:

1. Find clashing path names.
2. Iterate over values and if a path (computed from group/label) is not unique append a roman numeral corresponding to the current count for the non-unique path.

Previously we had a separate method which would merge existing Overlay items by trying to make their paths unique. This approach did not really work (again note the example above), and made the code more complex. The issue is that merging existing paths cannot be done correctly without trying to figure out which parts of the existing paths are roman numerals which is bound to be brittle. In effect the results should either be more correct or the same (as evidenced by most of the tests passing), unless you construct Layouts or Overlays with custom paths, which is presumably fairly rare. We do use custom paths in featuremapper/topographica but in those cases we use a Layout as a storage container and don't generally expect to merge them by using +.

[jlstevens]

unless you construct Layouts or Overlays with custom paths, which is presumably fairly rare

I'm not sure that is a safe assumption. Could you give a simple example of how it was supposed to work before, how it actually worked (assuming it was buggy) and how it works now?

My worry is that these changes could affect a lot of things and I want to understand the implications of this PR for backwards compatibility.

[philippjfr]

When inheriting and merging custom paths in the previous implementation it would follow the following rule, cut the path down to length 2, if the second part of the path matches the label keep it, otherwise drop it. So the difference basically comes down to whether the first part of the path, corresponding to the object's group, is inherited or not. I can probably still make that work now that I've factored things out into smaller functions and can make sense of it.

[philippjfr]

Okay I've reimplemented path inheritance which turned out to be much easier now that the code is factored into smaller chunks, some of which may be better as utilities. The behavior should now be the same as before except 1) numbering is correct and 2) it doesn't needlessly create a whole bunch of intermediate objects.

[jlstevens]

Looks good!

I'm happy to merge once the tests pass. I would also check that the new underscore methods don't show up when tab-completing on Layouts...

[philippjfr]

Still missing a lot of unit tests, perhaps I'll also make some of the underscore methods into utilities. Doubt they'll show up as we already had a bunch of underscored methods before.

[jlstevens]

perhaps I'll also make some of the underscore methods into utilities.

Up to you! Might make good sense although if you have a bunch or related utilities, it might be worth defining them as classmethods on a utility class.

[philippjfr]

@jlstevens Finally got around to adding some unit tests here, previous unit tests were only testing all the simple cases, which is how we missed this in the past. Ready to merge now when tests pass.

[jlstevens]

Looks good!

This PR took a while but I am glad this issue is now fixed. The code looks easier to understand than before with the bonus of actually being more correct.

Merging.

[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.