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

Fixed bug unpacking Overlay and Layout objects in Layout.from_values #1088

Merged
merged 14 commits into from Feb 23, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Jan 28, 2017

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

This comment has been minimized.

Member

philippjfr commented Jan 28, 2017

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

This comment has been minimized.

Member

philippjfr commented Jan 28, 2017

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.

@philippjfr philippjfr added the bug label Jan 28, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 29, 2017

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

This comment has been minimized.

Member

philippjfr commented Jan 29, 2017

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

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 10, 2017

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

This comment has been minimized.

Member

jlstevens commented Feb 10, 2017

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

This comment has been minimized.

Member

philippjfr commented Feb 10, 2017

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

This comment has been minimized.

Member

philippjfr commented Feb 11, 2017

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

This comment has been minimized.

Member

jlstevens commented Feb 12, 2017

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

This comment has been minimized.

Member

philippjfr commented Feb 12, 2017

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

This comment has been minimized.

Member

philippjfr commented Feb 13, 2017

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

This comment has been minimized.

Member

jlstevens commented Feb 13, 2017

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

This comment has been minimized.

Member

philippjfr commented Feb 13, 2017

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

This comment has been minimized.

Member

jlstevens commented Feb 13, 2017

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

This comment has been minimized.

Member

philippjfr commented Feb 23, 2017

@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

This comment has been minimized.

Member

jlstevens commented Feb 23, 2017

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.

@jlstevens jlstevens merged commit 9f777d3 into master Feb 23, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 78.261%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the overlay_fix branch Feb 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment