Add more descriptive pydeck types classes #4897

kylebarron · 2020-08-27T02:10:38Z

Background

Currently pydeck does a lot of "magic" behind the scenes to coerce user input into the representation sent to @deck.gl/json.

Strings are always treated as functions unless it includes an extra pair of quotes. So "properties.valuePerSqm" becomes the function datum => datum.properties.valuePerSqm. To pass a string literal, you need to use '"literal string"'.
When a literal string is passed, it removes all internal occurrences of that string. So '"literal "" string"' in Python becomes 'literal string' in JS.
Lists of strings are always treated as functions. In order to reference a literal string, you need to include another pair of quotes on that item.

Change List

These changes are designed to minimize backwards incompatibility.

Add helper classes that aim to be a 1:1 equivalent of what is possible in the JSON spec. These are named Enum, Literal, Function and Identifier.
Enum takes string input and prefixes @@# before serialization
Literal takes any JSON-encodable object and passes it unchanged to JS
Function takes varied input.
- For string input it prefixes @@= for serialization.
- For number input it stringifies the number and prefixes @@=.
- For Literal input it prefixes @@= and surrounds the input with double quotes. I.e. Literal('foo') becomes @@="foo". This is necessary because the @deck.gl/json docs say:
  
  Interpret the rest of the string as a function, parsing unquoted character strings as identifiers
- The Identifier class is included as a more-explicit form of string input. It includes its string unquoted.
- Each of these should also work within array input. So you should be able to pass Function([Identifier('x'), Identifier('y'), Literal(0)]) to render data at ground level.
When a literal string is passed in double quotes, remove only the first and last characters and leave the rest of the string intact. So '"literal "" string"' in Python becomes 'literal "" string' in JS. This is a backwards incompatible change, but the existing behavior is arguably broken.

coveralls · 2020-08-27T02:24:53Z

Coverage remained the same at 80.595% when pulling 538b223 on kyle/pydeck-type-classes into 0e9109a on master.

ajduberstein · 2020-08-27T07:23:45Z

bindings/pydeck/pydeck/bindings/types.py

+        try:
+            json.dumps(value)
+        except TypeError as e:
+            raise TypeError("Literal value must be JSON serializable\n" + e)


You're going to get a TypeError in your TypeError message, since the string and error don't concatenate. Maybe

raise TypeError("pydeck Literal value must be JSON serializable and input {} is not".format(value))

Yes you're right. I think I'm looking for e.message.

ajduberstein · 2020-08-27T07:24:21Z

bindings/pydeck/pydeck/bindings/types.py

+
+
+class Type(metaclass=abc.ABCMeta):
+    """Base class for Pydeck defined type"""


Nit: pydeck is lower-cased, like pandas or folium.

isaacbrodsky

I like the idea of having typed objects for these and not automagically deduce the encoding

isaacbrodsky · 2020-08-27T16:04:56Z

bindings/pydeck/pydeck/bindings/types.py

+ENUM_IDENTIFIER = "@@#"
+
+
+class Type(metaclass=abc.ABCMeta):


I would suggest naming this something more descriptive than "Type", which seems a little generic.

Since it's a base class, BaseType might be more apt?

Nit: Naming base classes with Base... is one my pet peeves. I feel that it is a design cop-out. Let's ideate a bit more first.

SerializableType, TransportType, TransportableType, ...

Right. This base class isn't public so I'm not too concerned with what the name should be, and welcome those suggestions. I'd also consider prefixing with _ to make clear it's private.

isaacbrodsky · 2020-08-27T16:06:15Z

bindings/pydeck/pydeck/bindings/layer.py

@@ -82,28 +83,29 @@ def __init__(self, type, data, id=None, use_binary_transport=None, **kwargs):
        self._kwargs = kwargs.copy()
        if kwargs:
            for k, v in kwargs.items():
+                if isinstance(v, Type):
+                    continue
+
                # We assume strings and arrays of strings are identifiers
                # ["lng", "lat"] would be converted to '[lng, lat]'
                # TODO given that data here is usually a list of records,
                # we could probably check that the identifier is in the row
                # Errors on case like get_position='-', however

                if isinstance(v, str) and v[0] in QUOTE_CHARS and v[0] == v[-1]:


This will also fail for "a" + b + "c" (results in a" + b + "c)

I think the question here is what is the desired behavior? Is "a" + b + "c" -> a + b + c more desired?

I would expect "a" + b + "c" to be "a" + b + "c" to match the behavior of "a" + "c" + b. The type result changing so dramatically from changing the order of operators is surprising.

Right I'd also prefer that but it's a breaking change.

ibgreen · 2020-08-27T17:26:35Z

Side note: don't want to make this more complex but I am thinking about how these design choices translate to the bigger cross-language picture.

E.g. if similar constructs would make sense in C++, Java, Swift Proxy bindings...

I really want these various API to develop in the same general direction.

Could be worth sketching out how things would look in those APIs...

kylebarron · 2020-08-27T17:29:11Z

I am thinking about how these design choices translate to the bigger cross-language picture.

I think it's good to have that discussion before merging a PR such as this one into Pydeck. I could envision a similar API for other languages. A good precursor might be to minimize the differences between pydeck and the JSON spec?

ibgreen · 2020-08-31T14:37:13Z

I think it's good to have that discussion before merging a PR such as this one into Pydeck. I could envision a similar API for other languages. A good precursor might be to minimize the differences between pydeck and the JSON spec?

Agreed.

About Proxy API definition:

At the moment we don't have a reference Proxy API for deck.gl JSON that other languages can just follow. The most "natural" proxy API would be implemented in JavaScript - but it is not really needed for obvious reasons (though it could potentially be useful for e.g. for rendering deck.gl from Node.js, and certainly for testing proxying setups).

We do have pydeck and experimental Java and Swift proxy APIs. We may also want to start sketching on a C++ proxy API.

Add more descriptive pydeck Layer types

538b223

kylebarron requested review from ajduberstein and ibgreen August 27, 2020 02:10

kylebarron mentioned this pull request Aug 27, 2020

Workaround for pydeck magic/array handling UnfoldedInc/earthengine-layers#97

Merged

ajduberstein reviewed Aug 27, 2020

View reviewed changes

isaacbrodsky reviewed Aug 27, 2020

View reviewed changes

ajduberstein mentioned this pull request Sep 28, 2020

Using local image file with BitmapLayer doesn't work - Pydeck #4977

Closed

ibgreen mentioned this pull request Oct 26, 2020

pydeck 0.6 Release Tracker #5063

Closed

4 tasks

donmccurdy added the pydeck label Mar 29, 2024

donmccurdy added this to the v9.0 milestone Mar 29, 2024

Pessimistress force-pushed the master branch 4 times, most recently from 0a083b6 to eb85da3 Compare April 25, 2024 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more descriptive pydeck types classes #4897

Add more descriptive pydeck types classes #4897

kylebarron commented Aug 27, 2020

coveralls commented Aug 27, 2020

ajduberstein Aug 27, 2020

kylebarron Aug 27, 2020

ajduberstein Aug 27, 2020

isaacbrodsky left a comment

isaacbrodsky Aug 27, 2020

kylebarron Aug 27, 2020

ibgreen Aug 27, 2020

kylebarron Aug 27, 2020

isaacbrodsky Aug 27, 2020

kylebarron Aug 27, 2020

isaacbrodsky Aug 27, 2020

kylebarron Aug 27, 2020

ibgreen commented Aug 27, 2020

kylebarron commented Aug 27, 2020

ibgreen commented Aug 31, 2020



		class Type(metaclass=abc.ABCMeta):
		"""Base class for Pydeck defined type"""

Add more descriptive pydeck types classes #4897

Are you sure you want to change the base?

Add more descriptive pydeck types classes #4897

Conversation

kylebarron commented Aug 27, 2020

Background

Change List

coveralls commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacbrodsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen commented Aug 27, 2020

kylebarron commented Aug 27, 2020

ibgreen commented Aug 31, 2020