Pull Request for multi-layer support #129

lucasvw · 2018-10-03T20:22:27Z

Hi there,

I just finished some refactoring for multi-layer support. I basically implemented a lower level Map class and all appropriate Layer classes. This was needed because the current *Viz classes are a combination of setting up the main Map instance as well as adding the (Source and) and Layers with the actual data. I am not a big fan of the Viz-collection-approach because each Viz instance can have its own specification of the actual Map (for example the "size" or "base-layer" etc). I think its more appropriate to get closer to the actual JS implementation and have Python classes for Map and Layers as well.

Most of the logic now happens in these lower Map and Layer classes, the *Viz classes just pass the logic onto these lower-level classes (Map and a single Layer). I didnt want to remove them for backwards compatibility but the idea for the future is that people start using the Map and Layer API's. I will try to add an example as soon as possible.

I have tested locally an example with multiple choroplethlayers and have run the examples contained in the project. I think some of the tests are broken so that has to be checked as well.

lucasvw · 2018-10-05T10:26:39Z

Hi @ryanbaumann,

I have seen there are multiple pull-requests for this issue so its probably a good idea to continue with only one of them. I still have time next week to put in some work but to prevent 'double-work' its perhaps best to decide which multi-layer extension has your/mapbox's preference.

Would be great to have some feedback!

akacarlyann · 2018-10-05T14:34:42Z

@lucasvw I like the idea of a layer-based api. Overall cleaner. Mine was definitely more of an experiment from a few months back. Curious to see how you handle layer ordering. Looking forward to checking it out!

lucasvw · 2018-10-05T16:15:18Z

Agreed, I also think its the right approach. I also kind of like the "json specs" approach although I have my doubts: (1) i guess the python api will never be as complete as the Javascript lib, its more a quick way to create visualizatios based on data that you wrangled in python.. if you need full control over the visualization, youll always need to get down to mapboxgl-js. (2) you probably cant get rid of javascript code snippets in this repo, I am not totally familiar with the expressiveness of the json spec but I guess you ll always need some JS glue to make everything work (can you declare legends for example in the spec?)

Currently havent done anything extra related to layer ordering, e.g. the belowLayer functionality is copied over to the Layer level.

Ill make an example with multilayers as soon as possible, probably sunday or monday.

akacarlyann · 2018-10-05T16:32:56Z

I think the JSON layer specification is still important to develop (#23) but I don't think it is mutually exclusive with using a layer-based API :)

And yup, agree it would be great to reduce JavaScript snippets in this particular project.

lucasvw · 2018-10-05T16:41:58Z

Great, sounds good. I might also just have to dive a little bit deeper into these style-sheets to understand it better and how it could be used.

ryanbaumann · 2018-10-07T14:28:32Z

Agree with @akacarlyann @lucasvw, this is a great implementation. Let's move forward with this approach for multi-layer management.

lucasvw · 2018-10-08T09:50:03Z

Great!

I just added a new example: examples/notebooks/multi-layer-example.ipynb

It shows the Map and Layer API as well as a multi-layer example.

Feedback is welcome =)

ryanbaumann · 2018-10-10T00:28:33Z

Nice! I'll check it out later this week. Thanks @lucasvw !

lucasvw · 2018-10-10T08:20:25Z

Cool.

To summarize:

layers.py contains all logic related to layers, there is an abstract layer class and all other layers inherit from this class
map.py contains a single Map class
Most logic from all the Viz-classes is removed. Viz-classes just create a Map instance under the hood as well as the associated Layer instance
I tried as good as possible to split attributes between either the Map class (size of div, center of map, basic legend layout etc) and the individual Layer classes (data and layer and legedn specific stuff) but have a look if it all makes sense.
To make sure that attribute assignment on the Viz classes still works (as I saw is often used in the examples) I pass this assignment to either the associated Map or Layer instance through overriding the __setattr__ method
The Map class has an array layers and an add_layer(), remove_layer() method
Creation on HTML and the template structure is also slighly changed: Layer templates no longer inherit from the base, main and map templates. There is a single template for the map and individual templates for the layers. Upon creation of the HTML (for .show() method ) the templates from Map and all individual layers are gathered and merged.
If there are more than one layers with legends the respective legend elements should be rendered below each other (created a new legend-container div)

Some interactive layer control (turn on/off layers) would perhaps be a good next step..?

ryanbaumann · 2018-10-10T17:34:31Z

Thanks for the architecture overview.

Some interactive layer control (turn on/off layers) would perhaps be a good next step..?

Agree, that would be a great addition in a new PR!

lucasvw · 2018-10-18T14:23:23Z

@ryanbaumann any updates on checking this?

vincentsarago · 2018-10-19T19:20:18Z

thanks @lucasvw, can't wait to see it live, @ryanbaumann let me know if you need help here

ryanbaumann · 2018-10-19T22:07:10Z

@vincentsarago added you to contributors - yes, please take this PR review if you have time. Thank you!

ryanbaumann · 2018-10-21T19:04:14Z

Just merged in #120 to master, which will cause a few conflicts on this branch. Excited to get this branch merged in - and it will be high time for a relase after that!

Next steps:

git rebase with master and push to this branch @lucasvw
Review PR @vincentsarago
Review PR @ryanbaumann
Address feedback and merge

emakarov

Nice work! left some comments for your reference.

emakarov · 2018-10-21T23:34:38Z

mapboxgl/layers.py

+    def create_html(self):
+        options = dict(
+            opacity=self.opacity,
+            belowLayer=self.below_layer,


I understand this is artefact from previous code, but CamelCase in python looks like an anti-pattern. Would be great if this can be avoided

I guess this is done because the variables end up in HTMl/JS files where camelCase is quite common. But actually the variables don't enter into JS, since they are just used as placeholder in the templating, so I guess you are right! It will take some time changing all the variable names though.. :)

emakarov · 2018-10-22T13:38:56Z

mapboxgl/layers.py

+    def get_layer_specific_variables(self, options):
+        """Update map template variables specific to heatmap visual"""
+        options.update(dict(
+            geojson_data=json.dumps(self.data, ensure_ascii=False),


here and in some other places, json.dumps potentially can raise Exception that some objects are not JSON serializable. So, I think it would be useful to pass a custom serializer class into Layer's init somehow.
And in some corresponding/or similar situations be able to assign custom serializer

Hmm interesting, Ill see if I can figure something out. Any suggestions are welcome!

Some suggestion
maybe, AbstractLayer can receive two types of data in __init__

dict

str (in this case, user is responsible for serialization as he wants)

__init__ can receive additional kwarg data_serializator_class which (if not None) will be used in json.dumps(data, cls=data_serializator_class)

json.dumps can be moved to separate function of base class, like def serialize_data which will check whether data has type str or dict and make serialization accordingly

this approach will also give user ability to override def serialize_data for example, to use another serialization library (for ex. nssjson or ultrajson) if he wants to..

emakarov · 2018-10-22T13:40:10Z

mapboxgl/layers.py

+                 legend_key_shape='circle',
+                 *args,
+                 **kwargs):
+        """Construct a Mapviz object


Docstring seems to be misleading. Why not Construct ClusteredCircleLayer object ?

Good catch, I still need to go to all the docstrings and update them.

emakarov · 2018-10-22T13:45:47Z

mapboxgl/templates/map.html


+<style type='text/css'>


probably it makes sense to separate CSS and JS code at least to different {% include %} tags

Yes, agreed

lucasvw · 2018-10-22T15:41:25Z

@emakarov Thanks a lot for your review and comments, much appreciated!

vincentsarago

Few general comments for now.
Also we should update the tests

vincentsarago · 2018-10-22T13:32:48Z

mapboxgl/map.py

@@ -0,0 +1,165 @@
+import codecs


I wonder if we could name this sub-module differently.
I'm afraid some people could do from mapboxgl import map which will override map function. That's said I don't have a better name.
maybe @perrygeo has some thought on that

vincentsarago · 2018-10-22T13:35:03Z

mapboxgl/layers.py

+
+from mapboxgl.utils import color_map, height_map
+from mapboxgl import templates
+from mapboxgl.utils import img_encode, numeric_map


I think we can merge all the .utils import in one line

vincentsarago · 2018-10-22T13:41:16Z

mapboxgl/viz.py

-
-GL_JS_VERSION = 'v0.49.0'
-
-
 class MapViz(object):


This is something I'm not sure to understand, to me MapViz should inherit from Map class because from what I understood, Viz are just a Map + Layer

and also I think there might be a better way to reduce code duplication

Well the more I think about the more I think we should maybe deprecate the Viz, for now it works but it also means there will be a lot of code duplication and it might result in a bit of a nightmare when we either update the Map or Layers classes.

cc @ryanbaumann @lucasvw

The Viz classes are just wrappers around certain Map + Layer combinations to offer backwards compatibility. MapViz doesn't inherit from Map but contains a Map

vincentsarago · 2018-10-22T20:00:13Z

mapboxgl/layers.py

+                 legend_text_numeric_precision=None,
+                 legend_key_shape='square',
+                 min_zoom=0,
+                 max_zoom=24,


Also noting, we should maybe rename min_zoom and max_zoom because it can be confusing between min_zoom/max_zoom for the map instance. layer_min_zoom might be better

lucasvw · 2018-10-24T17:56:56Z

Hi everyone, thanks for all the feedback! I just started a new position and Iam kind of busy with that.. so I am afraid it will take a little until I have spare-time to spend on this. Feel free to fork and continue if somebody has earlier availability!

ryanbaumann · 2018-11-27T15:21:10Z

This is a huge feature and very close @lucasvw - do you have time to finish off for the coming release at the end of December 2019? If not, no worries - let's transfer ownership.

lucasvw · 2018-11-27T20:29:12Z

Sorry man, no time. As stated before, feel free to fork and continue from where I left off.

emakarov · 2019-01-14T08:24:10Z

so, what is the current status for this PR? seems like the desired feature is not going forward...

ryanbaumann · 2019-01-14T16:03:01Z

We'll need to find a new owner for this PR @emakarov to finish off. It is the top-requested feature at the moment for mapboxgl-jupyter.

emakarov · 2019-01-30T19:05:39Z

For those who interested in alternative (simple, yet flexible) multi-layer lib:

https://github.com/emakarov/mapboxgl_notebook

akacarlyann · 2019-01-30T22:03:35Z

@emakarov Nice! I especially like the way you formulated a Layout class.

ryanbaumann · 2019-01-30T22:11:58Z

Agreed @emakarov your implementation is an excellent approach. I'd love to see if we can adopt a similar approach in mapboxgl-jupyter to take advantage of all the other features that are built into this library (raster layers, plugins, helper functions, etc).

I will cut a few tickets to capture this issue and link back to this PR. Let's continue the discussion on the issues.

refactored

2d049d0

lucasvw added 4 commits October 8, 2018 11:03

bugfix Heatmap Viz/Layer

deabef1

Heatmap bugfix

ae76bda

refactored templates to prevent async problems with variable assignment

1f77287

added multilayer example

e2dc343

ryanbaumann added the ready for review label Oct 18, 2018

ryanbaumann requested a review from vincentsarago October 21, 2018 19:02

ryanbaumann assigned lucasvw Oct 21, 2018

This was referenced Oct 21, 2018

[WIP] Support of multiple layers. #126

Closed

Map collections #128

Closed

emakarov reviewed Oct 22, 2018

View reviewed changes

vincentsarago reviewed Oct 22, 2018

View reviewed changes

akacarlyann mentioned this pull request Nov 27, 2018

Variable radius legend #135

Merged

rmcarthur mentioned this pull request Jul 29, 2019

Mapboxgl viz types roadmap #4

Open

17 tasks


		<style type='text/css'>

Pull Request for multi-layer support #129

Are you sure you want to change the base?

Pull Request for multi-layer support #129

Conversation

lucasvw commented Oct 3, 2018 • edited Loading

lucasvw commented Oct 5, 2018

akacarlyann commented Oct 5, 2018

lucasvw commented Oct 5, 2018 • edited Loading

akacarlyann commented Oct 5, 2018 • edited Loading

lucasvw commented Oct 5, 2018 via email • edited Loading

ryanbaumann commented Oct 7, 2018

lucasvw commented Oct 8, 2018

ryanbaumann commented Oct 10, 2018

lucasvw commented Oct 10, 2018

ryanbaumann commented Oct 10, 2018

lucasvw commented Oct 18, 2018

vincentsarago commented Oct 19, 2018

ryanbaumann commented Oct 19, 2018

ryanbaumann commented Oct 21, 2018 • edited Loading

Next steps:

emakarov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasvw commented Oct 22, 2018

vincentsarago 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

lucasvw commented Oct 24, 2018

ryanbaumann commented Nov 27, 2018 • edited Loading

lucasvw commented Nov 27, 2018

emakarov commented Jan 14, 2019

ryanbaumann commented Jan 14, 2019 • edited Loading

emakarov commented Jan 30, 2019

akacarlyann commented Jan 30, 2019

ryanbaumann commented Jan 30, 2019

lucasvw commented Oct 3, 2018 •

edited

Loading

lucasvw commented Oct 5, 2018 •

edited

Loading

akacarlyann commented Oct 5, 2018 •

edited

Loading

lucasvw commented Oct 5, 2018 via email •

edited

Loading

ryanbaumann commented Oct 21, 2018 •

edited

Loading

ryanbaumann commented Nov 27, 2018 •

edited

Loading

ryanbaumann commented Jan 14, 2019 •

edited

Loading