Pull Request for multi-layer support

lucasvw commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

lucasvw commented 5 years ago

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 commented 5 years ago

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

lucasvw commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@ryanbaumann any updates on checking this?

vincentsarago commented 5 years ago

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

ryanbaumann commented 5 years ago

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

ryanbaumann commented 5 years ago

Just merged in https://github.com/mapbox/mapboxgl-jupyter/pull/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

lucasvw commented 5 years ago

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

lucasvw commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

emakarov commented 5 years ago

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

ryanbaumann commented 5 years ago

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 commented 5 years ago

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

https://github.com/emakarov/mapboxgl_notebook

akacarlyann commented 5 years ago

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

ryanbaumann commented 5 years ago

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.

mapbox / mapboxgl-jupyter

Pull Request for multi-layer support #129

Next steps: