-
Notifications
You must be signed in to change notification settings - Fork 5
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
Two-dimensional scatter plot #317
Conversation
'hist': hist, | ||
} | ||
for array in (values, mask): | ||
array['y'] = np.concatenate([array['y'][0:1], array['y']]) |
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.
What changed here? Before we didn't add a point to the values, but now we do.
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 what changed is that before we were using scipp.concat
for y values, and numpy
for the mask values, but now we switch to numpy earlier (because I had to resort to using np.asarray
because in some cases some lists of arrays were supplied)
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.
Aha yeah I see what you mean
HBox( | ||
[ | ||
self.left_bar, | ||
VBox([self._view.canvas.to_widget()]), |
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 don't understand what this does, why is the widget wrapped in a VBox
?
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 was an annoying thing where the buttons would end up all the way to the right, and for some reason, wrapping it in a VBox
helps.
However, I now see that's it's still a little broken (see #323).
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 i fixed it.
src/plopp/graphics/colormapper.py
Outdated
@@ -227,9 +227,10 @@ def _set_normalizer_limits(self): | |||
self.normalizer.vmin = self.vmin | |||
self.normalizer.vmax = self.vmax | |||
|
|||
def update(self, key: str, data: sc.DataArray): | |||
def update(self, _=None, **kwargs): |
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.
(minor) The argument name confused me here, I always assume _
is unused, so I was surprised when it was used.
Maybe we should rename it to values
?
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 basically wanted to avoid a name clash from happening. Like when you update a dict in python.
You can do
d.update(a=1, b=3, values=5)
d.update({'a': 1, 'b': 3, 'values': 5})
If my function signature is
def update(self, values=None, **kwargs):
then the first case will fail because it is expecting values
to be a dict, not just a single value.
I don't actually know how python does it, because you can also use _
as a key:
d = {'a': 1, 'b': 3}
d.update(_=55)
print(d)
d.update({'a': 33, 'c': 6})
print(d)
{'a': 1, 'b': 3, '_': 55}
{'a': 33, 'b': 3, '_': 55, 'c': 6}
Maybe I should look at how they implemented it...
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.
So I think a better way of implementing this is
def update(self, *args, **kwargs):
new = kwargs
if args:
if len(args) > 1:
raise TypeError('Update expected at most 1 arg. Got {}.'.format(len(args)))
else:
new.update(args[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.
Yeah that makes sense. We can avoid clashes and make the name more clear by using something like: _dict_arg
. That's unlikely to cause a clash and it's also a bit more clear. But I'll leave it to you to decide.
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.
Even with *args
there is no clash, you can still do view.update(args=1, a=2)
and the keys will be args
and a
.
src/plopp/graphics/graphicalview.py
Outdated
|
||
|
||
class GraphicalView(View): | ||
""" """ |
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 this needs a short docstring explaining what makes it different from View
.
The id of the node that sent the new data. | ||
""" | ||
if new_values.ndim != 1: | ||
raise ValueError("LineView can only be used to plot 1-D data.") |
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.
Maybe I missed something but it seems we loose this error message here. Is there another check elsewhere using the new _ndim
attribute?
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, at the start of the update
function in the GraphicalView
;-)
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 good to me. Tested the scatter plot and played around with it a bit and it seems to work well. The changes to the update
method and the to the structure seem to make a lot of sense.
The superplot was not interactive for me, do I need another package to make it work?
Did you use the notebook from the docs? Did you have |
Yes I did. But when running it the first time it told me to install |
I think you need to skip the weird cell with from ipywidgets import HBox
b = p.children[0].children[1].children[0]
sl = p.children[1].controls['y']['slider']
b.click()
sl.value = 20
b.click()
sl.value = 30
f = p.children[0].children[0]
f.children = [
f.top_bar,
HBox([f.left_bar, f.canvas.to_image(), f.right_bar]),
f.bottom_bar,
] which is hidden in the docs. |
Yes that did the trick. How do you make a cell hidden in the docs? |
|
This PR introduces a new 2d scatter plot.
Fixes #264
Example:
See docs notebook for more examples.
In addition, there has been a refactor of the interface to how we update figures:
Old:
New:
It made sense to do this change here to unify the different views (made a common
GraphicalView
base class).Also fixes #315
Also fixes #323