-
Notifications
You must be signed in to change notification settings - Fork 363
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
base: master
Are you sure you want to change the base?
Adapt velocity layer for an easier use with a ColormapControl #1035
Conversation
"# Set up for JupyterLite\n", | ||
"try:\n", | ||
" import piplite\n", | ||
" await piplite.install(['ipyleaflet')\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" await piplite.install(['ipyleaflet')\n", | |
" await piplite.install('ipyleaflet')\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You reintroduced the [
.
ipyleaflet/velocity.py
Outdated
@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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 scale
as 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 )
Thanks @HaudinFlorence! |
1d01783
to
ed48e77
Compare
The docstrings have been updated. |
js/src/layers/Velocity.js
Outdated
@@ -25,7 +25,7 @@ export class LeafletVelocityModel extends layer.LeafletLayerModel { | |||
minVelocity: 0, | |||
maxVelocity: 10, | |||
velocityScale: 0.005, | |||
colorScale: [] | |||
_colorScale: [] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ).
…olormap of Branca. This enables to easily use a ColormapControl with this layer.
Co-authored-by: David Brochart <[email protected]>
Co-authored-by: David Brochart <[email protected]>
Co-authored-by: David Brochart <[email protected]>
…in Velocity.js, change default colormap to OrRd_06 to fit other default values in ipyleaflet, update docstrings.
…bserved in Velocity.js.
…ly a colormap subitem attached to the velocity layer.
32af15e
to
6dec92f
Compare
5d0afd1
to
7c5a482
Compare
ipyleaflet/velocity.py
Outdated
color_scale = List([]).tag(sync=True, o=True) | ||
caption = Unicode("").tag(sync=True, o=True) |
There was a problem hiding this comment.
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?
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) |
"rgb(220,24,32)", | ||
"rgb(180,0,35)" | ||
]).tag(sync=True, o=True) | ||
colormap = Any(linear.OrRd_06) |
There was a problem hiding this comment.
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)
Co-authored-by: martinRenou <[email protected]>
Co-authored-by: martinRenou <[email protected]>
Co-authored-by: martinRenou <[email protected]>
…nt be displayed with the current version of the branca colormap.
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.