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

Merging additions in ecoscope fork #592

Open
atmorling opened this issue Aug 12, 2024 · 7 comments
Open

Merging additions in ecoscope fork #592

atmorling opened this issue Aug 12, 2024 · 7 comments

Comments

@atmorling
Copy link

Things I've added in our fork that may be worth adding/reviewing here

Things that are simple(ish) to merge

  • v9 bump
  • Add support for Deck widgets
  • Add mapWidth to Map
  • An alternative viz that returns a layer rather than a map
  • Surfacing deck.gl MapView in index.tsx - we're doing this to allow repeat=true
  • Surface controller Python side in Map

Less simple

  • Supporting custom widgets (non deck.gl native js components). It's easy enough to add custom components on the python side, and it's easy enough to split our custom js components into an NPM package. What's unclear to me is the best way to tie a custom js package into lonboard and if that's even a good idea.
@batpad
Copy link
Member

batpad commented Aug 12, 2024

+cc @vgeorge for visibility

@vgeorge vgeorge mentioned this issue Aug 27, 2024
6 tasks
@kylebarron
Copy link
Member

See #613 in which @vgeorge started a PR to add your additions to lonboard.

  • An alternative viz that returns a layer rather than a map

In terms of API design, what do you think about just documenting that you can access the generated layers via

from lonboard import viz

viz_layers = viz(data).layers

Then we don't need another top-level function just to create layers.

  • Surface controller Python side in Map

It looks like you made this boolean? So you didn't have a use case of customizing the controller; just of turning off interaction?

  • Add support for Deck widgets

I see that all of your deck widgets subclass from lonboard.base.BaseWidget. For some of the widgets that don't need jupyter-based interactivity, I think it might be better if they aren't their own Jupyter widgets, but rather you just pass in props to the top-level Map.

I have to think about this a little more.

@atmorling
Copy link
Author

In terms of API design, what do you think about just documenting that you can access the generated layers via

from lonboard import viz

viz_layers = viz(data).layers

In a world where I already have a map with some layers, and I want to add another layer based on the output of viz (ie I have a mixed geometry GDF, but specific styling needs for each geometry type), it would be nice to not have to create another map entirely. I guess it depends if you see that being a typical use case or not.

It looks like you made this boolean? So you didn't have a use case of customizing the controller; just of turning off interaction?

Yep

  • Add support for Deck widgets

I see that all of your deck widgets subclass from lonboard.base.BaseWidget. For some of the widgets that don't need jupyter-based interactivity, I think it might be better if they aren't their own Jupyter widgets, but rather you just pass in props to the top-level Map.

I have to think about this a little more.

On a slightly related note, the way those are implemented also duplicates getChildModelState with getDeckWidgetModelState for updating the (deck) widget state in index.tsx.
I keep changing my opinion on whether it's better that way or to just roll everything into getChildModelState and have it figure out if childModel is a widget or layer from a traitlet or similar.

@kylebarron
Copy link
Member

In a world where I already have a map with some layers, and I want to add another layer based on the output of viz (ie I have a mixed geometry GDF, but specific styling needs for each geometry type), it would be nice to not have to create another map entirely. I guess it depends if you see that being a typical use case or not.

Well, we could add an add_layer method to Map

def add_layer(self, layer: BaseLayer | Sequence[BaseLayer] | Map):
    if isinstance(layer, Map):
        layer = layer.layers
	...

And then you could do

from lonboard import viz

first_map = viz(...)
first_map.add_layer(viz(other_data))

Separately, we should probably store the list of layers as a tuple under the hood, so that changing the list of layers forces it to be a new object, so that traitlets knows to sync it to the frontend.

@atmorling
Copy link
Author

I like that. I've adopted something similar within ecoscope to manage the layers, though I didn't realise that tuples were treated that way in traitlets 🤯
I've been copy()ing the list to provide a 'new' object which forces an update. There's also spectate which seems a bit heavyweight.

@kylebarron
Copy link
Member

kylebarron commented Sep 3, 2024

Well, as I understand it, traitlets can observe when a new Python object is provided, but not if the object has mutated. That is, it checks for equality using is. If you store data in mutable data types, like list, then it's easy to mutate the object instead of providing a new one. The only thing special about a tuple is that it's immutable, so you can't accidentally mutate in. We should really be storing all internal sequence traits as tuples.

There's also spectate which seems a bit heavyweight.

spectate is old and unmaintained. psygnal is modern, maintained, and already a dependency (via anywidget) but I never figured out how to use it. https://psygnal.readthedocs.io/en/stable/dataclasses/, #210

kylebarron added a commit that referenced this issue Sep 3, 2024
Ref
#592 (comment)

This should be easier to follow for users. Mutating a list trait doesn't
propagate any updates to the frontend. Rather, you must set a new
object. Storing tuple traits instead of list traits means that it's
impossible for the user to accidentally not sync those updates to the
frontend.
@kylebarron
Copy link
Member

#620 now stores internal sequences as lists and added a add_layer method to the map object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants