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

Adapt velocity layer for an easier use with a ColormapControl #1035

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

HaudinFlorence
Copy link
Contributor

Adapt velocity layer re-building the color_scale list from a linear colormap of Branca. This enables to easily use a ColormapControl with this layer using the same colormap in the layer and in the control.

"# Set up for JupyterLite\n",
"try:\n",
" import piplite\n",
" await piplite.install(['ipyleaflet')\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
" await piplite.install(['ipyleaflet')\n",
" await piplite.install('ipyleaflet')\n",

Copy link
Member

Choose a reason for hiding this comment

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

You reintroduced the [.

Comment on lines 73 to 92
@default('color_scale')
def _default_color_scale(self):
self.color_scale = []

for color in self.colormap.colors:
rgb_tuple = tuple(int(x * 256) for x in color[:3])
rgb_str = f"rgb{rgb_tuple}"
self.color_scale.append(rgb_str)

return self.color_scale
Copy link
Member

Choose a reason for hiding this comment

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

By default we don't have the same color scale as before, maybe we should?
Also, it might be good to check that we don't pass a colormap and a color_scale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right that we don't have the same default color scaleas before. Here we propose to define color_scale from colormap in such a way that there is a total correspondance between the mapping of colors in the colormap and the mapping of colors on the map (i.e. the color_scale). To avoid any confusion, we propose that the user only has to give a colormap and we change color_scale into a private attribute (change proposed in ed48e77 )

@davidbrochart
Copy link
Member

Thanks @HaudinFlorence!
It could be good to update the documentation also?

@HaudinFlorence HaudinFlorence force-pushed the adapt_velocity_layer_for_an_easier_use_with_a_ColormapControl branch from 1d01783 to ed48e77 Compare August 12, 2022 10:01
@HaudinFlorence
Copy link
Contributor Author

Thanks @HaudinFlorence! It could be good to update the documentation also?

The docstrings have been updated.

@@ -25,7 +25,7 @@ export class LeafletVelocityModel extends layer.LeafletLayerModel {
minVelocity: 0,
maxVelocity: 10,
velocityScale: 0.005,
colorScale: []
_colorScale: []
Copy link
Member

Choose a reason for hiding this comment

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

Actually the VelocityLayer expects colorScale, so this won't work, unless you copy the value of _colorScale to the colorScale option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have reverted the change of making color_scale a private attribute (32af15e ).

HaudinFlorence and others added 8 commits September 23, 2022 16:44
…olormap of Branca. This enables to easily use a ColormapControl with this layer.
Co-authored-by: David Brochart <david.brochart@gmail.com>
Co-authored-by: David Brochart <david.brochart@gmail.com>
Co-authored-by: David Brochart <david.brochart@gmail.com>
…in Velocity.js, change default colormap to OrRd_06 to fit other default values in ipyleaflet, update docstrings.
…ly a colormap subitem attached to the velocity layer.
@HaudinFlorence HaudinFlorence force-pushed the adapt_velocity_layer_for_an_easier_use_with_a_ColormapControl branch from 32af15e to 6dec92f Compare September 23, 2022 15:35
@HaudinFlorence HaudinFlorence force-pushed the adapt_velocity_layer_for_an_easier_use_with_a_ColormapControl branch from 5d0afd1 to 7c5a482 Compare September 23, 2022 15:53
ipyleaflet/velocity.py Outdated Show resolved Hide resolved
ipyleaflet/velocity.py Outdated Show resolved Hide resolved
Comment on lines 80 to 81
color_scale = List([]).tag(sync=True, o=True)
caption = Unicode("").tag(sync=True, o=True)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to sync this one traits with the front-end?

Suggested change
color_scale = List([]).tag(sync=True, o=True)
caption = Unicode("").tag(sync=True, o=True)
color_scale = List([]).tag(sync=True, o=True)
caption = Unicode("").tag(o=True)

js/src/layers/Velocity.js Outdated Show resolved Hide resolved
"rgb(220,24,32)",
"rgb(180,0,35)"
]).tag(sync=True, o=True)
colormap = Any(linear.OrRd_06)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to be able to change this trait dynamically

velocity_layer.colormap = linear.Accent_04

(same comment for caption)

HaudinFlorence and others added 4 commits September 26, 2022 09:39
Co-authored-by: martinRenou <martin.renou@gmail.com>
Co-authored-by: martinRenou <martin.renou@gmail.com>
Co-authored-by: martinRenou <martin.renou@gmail.com>
…nt be displayed with the current version of the branca colormap.
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

Successfully merging this pull request may close these issues.

None yet

3 participants