Skip to content
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

Add more descriptive pydeck types classes #4897

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion bindings/pydeck/pydeck/bindings/json_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
"""
import json

from .types import Type

# Attributes to ignore during JSON serialization
IGNORE_KEYS = [
"mapbox_key",
Expand Down Expand Up @@ -60,16 +62,25 @@ def lower_camel_case_keys(attrs):
camel_key = camel_and_lower(snake_key)
attrs[camel_key] = attrs.pop(snake_key)

return attrs


def default_serialize(o, remap_function=lower_camel_case_keys):
"""Default method for rendering JSON from a dictionary"""
attrs = vars(o)

# Remove keys where value is None
attrs = {k: v for k, v in attrs.items() if v is not None}
for ignore_attr in IGNORE_KEYS:
if attrs.get(ignore_attr):
del attrs[ignore_attr]

# For each object of type pydeck.Type, call .json
attrs = {k: v.json() if isinstance(v, Type) else v for k, v in attrs.items()}

if remap_function:
remap_function(attrs)
attrs = remap_function(attrs)

return attrs


Expand Down
34 changes: 18 additions & 16 deletions bindings/pydeck/pydeck/bindings/layer.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import uuid
from collections.abc import Iterable

import numpy as np

from ..data_utils import is_pandas_df, has_geo_interface, records_from_geo_interface
from ..data_utils import has_geo_interface, is_pandas_df, records_from_geo_interface
from .json_tools import JSONMixin, camel_and_lower

from .types import Function, Literal, Type

TYPE_IDENTIFIER = "@@type"
FUNCTION_IDENTIFIER = "@@="
Expand Down Expand Up @@ -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]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Skip quoted strings
kwargs[k] = v.replace(v[0], "")
# For quoted strings, consider inner part a literal string
kwargs[k] = Literal(v[1:-1])

elif isinstance(v, str):
# Have @deck.gl/json treat strings values as functions
kwargs[k] = FUNCTION_IDENTIFIER + v

elif isinstance(v, list) and v != [] and isinstance(v[0], str):
# Allows the user to pass lists e.g. to specify coordinates
array_as_str = ""
for i, identifier in enumerate(v):
if i == len(v) - 1:
array_as_str += "{}".format(identifier)
else:
array_as_str += "{}, ".format(identifier)
kwargs[k] = "{}[{}]".format(FUNCTION_IDENTIFIER, array_as_str)
# Treat strings values as functions
kwargs[k] = Function(v)

elif isinstance(v, Iterable) and v != [] and isinstance(v[0], str):
# Allow lists of str e.g. to specify coordinates
kwargs[k] = Function(v)

else:
kwargs[k] = Literal(v)

self.__dict__.update(kwargs)

Expand Down
101 changes: 101 additions & 0 deletions bindings/pydeck/pydeck/bindings/types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import abc
import json
from collections.abc import Iterable

TYPE_IDENTIFIER = "@@type"
FUNCTION_IDENTIFIER = "@@="
ENUM_IDENTIFIER = "@@#"


class Type(metaclass=abc.ABCMeta):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Base class for Pydeck defined type"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def __init__(self, value):
self._value = value

def __repr__(self):
return self.type_name + "(" + str(self.json()) + ")"

@property
@abc.abstractmethod
def type_name(self):
"""Name of Type; used in default __repr__"""

@abc.abstractmethod
def json(self):
"""JSON representation of object for serialization"""


class Enum(Type):
"""Enum type

Interpret as an Enum, resolving in the JSON configuration
"""

type_name = "Enum"

def __init__(self, value):
if not isinstance(value, str):
raise TypeError("Enum value must be a string")

super(Enum, self).__init__(value)

def json(self):
return ENUM_IDENTIFIER + self._value


class Function(Type):
"""Function type

Interpret as a function, parsing unquoted character strings as identifiers
"""

type_name = "Function"

def json(self):
if isinstance(self._value, Iterable) and not isinstance(self._value, str):
return FUNCTION_IDENTIFIER + "[{}]".format(", ".join(map(str, self._value)))

return FUNCTION_IDENTIFIER + str(self._value)


class Identifier(Type):
"""Identifier type
"""

type_name = "Identifier"

def __init__(self, value):
if not isinstance(value, str):
raise TypeError("Identifier value must be a string")

super(Identifier, self).__init__(value)

def __str__(self):
return str(self._value)

def json(self):
return self._value


class Literal(Type):
"""A Literal object

Can be any JSON-encodable object
"""

type_name = "Literal"

def __init__(self, value):
try:
json.dumps(value)
except TypeError as e:
raise TypeError("Literal value must be JSON serializable\n" + e)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


super(Literal, self).__init__(value)

def __str__(self):
return '"{}"'.format(self._value)

def json(self):
return self._value