Fix bug in reconstructing layered charts with from_json/from_dict

[binste]

Fixes #3067. When reconstructing a layered chart which has heightor width defined on the top-level with from_dict, the code so far checked that the layers have that property set to Undefined or to the same value as the layered chart itself. However, it's often the case that the layers don't have the property set at all -> This fix handles those cases.

I also added a test to run a to_json and from_json roundtrip for all examples in the gallery so that we can hopefully catch future regressions to this functionality. I checked that these tests would have caught the bug reported in #3067.

@mattijn Would be great if this fix can still make it into 5.0.1 in case you or someone else has the time to review it.

[mattijn]

Thanks for this PR @binste!

On the branch of this PR 23 examples (arguments syntax only) are failing your introduced test without the patch you suggested. I applied the same test (arguments syntax only) to the 4.2.x branch and there all tests pass (0 fails) without requiring your suggested patch.

This makes we wonder if we can discover why it is currently often the case that the layers don't have the property set at all. Another thing that makes me wonder, I always thought that -all- properties by default are set to Undefined, so in my mind it is or defined or Undefined, but not nothing. See any API reference (eg alt.LayerChart).

The following are just guesses, buy do we maybe miss something in the codegen.py/generate_schema_wrapper.py? There we append =Undefined at L209 , L252 and L520, but is that all required? Or maybe something related to the UndefinedType (around here)?

[binste]

The tests pass in Altair 4 as the behavior of lifting height and width to the top-level of a layered chart were introduced in Altair 5 in c2a58b2 and with it also the line of code which I patched.

For a normal Chart, they would be set to Undefined:

import altair as alt
c = alt.Chart().mark_bar()
c._kwds

{'data': Undefined,
 'mark': 'bar',
 'align': Undefined,
 'autosize': Undefined,
 'background': Undefined,
 'bounds': Undefined,
 'center': Undefined,
 'config': Undefined,
 'datasets': Undefined,
 'description': Undefined,
 'encoding': Undefined,
 'height': Undefined,
 'name': Undefined,
 'padding': Undefined,
 'params': Undefined,
 'projection': Undefined,
 'resolve': Undefined,
 'spacing': Undefined,
 'title': Undefined,
 'transform': Undefined,
 'usermeta': Undefined,
 'view': Undefined,
 'width': Undefined}

But I think it makes sense that height and width are not defined at all in the layers when reconstructing it as the layers are following the specifications of a UnitSpec which does not have those attributes.

layered_reconstructed.layer[0]._kwds:

{'mark': MarkDef({
   type: Mark('bar')
 }),
 'data': NamedData({
   name: 'empty'
 }),
 'description': Undefined,
 'encoding': Undefined,
 'name': Undefined,
 'params': Undefined,
 'projection': Undefined,
 'title': Undefined,
 'transform': Undefined}

This is also visible in the Atlassian JSON Schema Viewer for a LayerSpec -> layers shows that it accepts an array of LayerSpec or GenericUnitSpec<Encoding,AnyMark>. GenericUnitSpec does not accept width or height, see here.

In my understanding, when converting to a Vega-Lite spec it doesn't matter if an attribute is not defined at all in the Altair objects or set to Undefined as we exclude all key, value pairs with value Undefined when converting to a dictionary, see here.

Would you agree or do you think there is something we should still dig deeper into?

[mattijn]

Thanks for the clarification @binste! It is more clear now. I thought with 'not set' it meant that it was a valid property for the specific component, but it was not given as defined or Undefined, but as you explained, this GenericUnitSpec does not accept these properties. Merging this. Thanks again!