-
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 heatmap layer to be used with a colormap control #1036
base: master
Are you sure you want to change the base?
Adapt heatmap layer to be used with a colormap control #1036
Conversation
97a77e7
to
0a396c7
Compare
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.
Looks like you should rebase (79e7d94 is already in master).
ipyleaflet/leaflet.py
Outdated
colors = ['blue', 'cyan', 'lime', 'yellow', 'red'] | ||
values = [0.4, 0.6, 0.7, 0.8, 1.0] |
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 might want to use traits for these, otherwise they are not configurable.
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.
Thanks a lot for the review @davidbrochart. Logics of the attributes has been changed a little bit in 93fa3c4, but the choice of the values and colors are configurable using gradient, as shown in the the example notebook Heatmap_with_colormap.
ipyleaflet/leaflet.py
Outdated
dict = {} | ||
for i in range(len(values)): | ||
dict[values[i]] = colors[i] | ||
vmin = values[0] | ||
vmax = values[len(values) - 1] | ||
gradient = Dict(dict).tag(sync=True, o=True) | ||
colormap = LinearColormap(colors, vmin=vmin, vmax=vmax) |
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.
vmin
, vmax
, gradient
and colormap
should be computed when values
or colors
change.
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.
This has been tried in commit 93fa3c4.
…vmax and colormap from gradient data (values and colors).
0a396c7
to
93fa3c4
Compare
This has been fixed. |
ipyleaflet/leaflet.py
Outdated
colors = list(self.gradient.values()) | ||
self.vmin = values[0] | ||
self.vmax = values[len(values) - 1] | ||
self.colormap = LinearColormap(colors, vmin=self.vmin, vmax=self.vmax) |
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.
With the way gradient
is defined, is it really a linear color map?
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.
Update docstrings Co-authored-by: David Brochart <[email protected]>
Co-authored-by: David Brochart <[email protected]>
…ar colormap with custom boundary colors and values.
ipyleaflet/leaflet.py
Outdated
self.vmin = values[0] | ||
self.vmax = values[-1] |
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.
I think we need to sort the gradient by value first.
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.
Sorry. I am not sure to understand.
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.
gradient
is a dict of "value:color" entries. I don't think we can expect the first entry to have the minimum value and the last entry to have the maximum value.
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.
Thanks for pointing this out. The name values
was indeed not a good choice, since it actually refers to the key of the gradient
dict. New naming colormap_labels
is proposed in 27acdac
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.
I meant that we need to sort the colormap_labels
. What happens to vmin
and vmax
if gradient = {1.0: 'red', 0.6: 'cyan', 0.7: 'lime', 0.8: 'yellow', 0.4: 'blue'}
?
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.
I meant that we need to sort the
colormap_labels
. What happens tovmin
andvmax
ifgradient = {1.0: 'red', 0.6: 'cyan', 0.7: 'lime', 0.8: 'yellow', 0.4: 'blue'}
?
You're right. Branca colormap requires that the colormaps_labels
are sorted. This point has been solved in commit 373f5d9
The example notebook has also been modified for a case where the gradient is initially not sorted.
Co-authored-by: David Brochart <[email protected]>
Adapt heatmap layer for an easier use with a colormap control. The colormap is defined from the color gradient defined in Heatmap class
.