Bokeh plotter #714
Bokeh plotter #714
Conversation
Also includes - ctapipe.utils.rgbtohex - ctapipe.visualization.bokeh - ctapipe.plotting.bokeh_event_viewer
* master: Copy changes to a Factory's trait default value to its subclasses (cta-observatory#713)
|
An animation of the file_viewer (some updates have been made since this gif's creation) |
|
Looks good. Will checkout the code later. 👍 |
| exe.run() | ||
|
|
||
|
|
||
| if 'bk_script' in __name__: |
There was a problem hiding this comment.
this is very unusual. What is this for?
There was a problem hiding this comment.
To run a bokeh server you have to execute bokeh serve on the script instead of python. When you do this, __name__ equals something like "bk_script_dc88af7ef51b4e0d85854896dd662ed0", where the third term seems to be random.
There was a problem hiding this comment.
However, there may be a way to avoid having to use bokeh serve if I can understand how to utilise this: http://bokeh.pydata.org/en/latest/docs/user_guide/server.html#embedding-bokeh-server-as-a-library
Codecov Report
@@ Coverage Diff @@
## master #714 +/- ##
==========================================
+ Coverage 71.93% 73.33% +1.39%
==========================================
Files 204 211 +7
Lines 11061 12054 +993
==========================================
+ Hits 7957 8840 +883
- Misses 3104 3214 +110
Continue to review full report at Codecov.
|
ctapipe/utils/rgbtohex_c.cc
Outdated
| #include <stdint.h> | ||
| #include <stdio.h> | ||
|
|
||
| extern "C" void rgbtohex(const uint8_t* rgb, size_t n_pix, char* hex) |
There was a problem hiding this comment.
TL;DR: I believe in this particular case the C-extension is not necessary.
I believe we should avoid C-extensions as long as possible and I assume you believe so too (as opposed to writing C-extension just for fun). So I assume you had no chance to avoid this, since with pure Python this was performing horrible. In such a case I believe C-extensions are fine.
Still, I personally consider writing a C-extension so exceptional that a little discussion of it might not be too annoying. I would have liked the author to present maybe some numbers in a comment or in the PR-discussion to backup the decision to go for a C-extension.
Since these numbers were missing, I tried to understand myself what was so slow here. For this I created a Python implementation of what I understood the C-implementation to do and profiled it by hand (using simply %timeit in my ipython session).
The result was a second implementation of intensity_to_hex which is simply named intensity_to_hex2 which is pure python and happens to be even faster than the version using the C-extension.
Here is the comparison:
In [1]: from rgbtohex import *
In [2]: array = np.random.rand(10000)
In [3]: assert (intensity_to_hex(array) == intensity_to_hex2(array)).all()
In [4]: %timeit intensity_to_hex(array)
2.99 ms ± 13.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
In [5]: %timeit intensity_to_hex2(array)
1.75 ms ± 10.6 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
This is the original intensity_to_hex (rewritten a bit and added the runtimes per line):
def intensity_to_hex(array, minval=None, maxval=None):
hex_ = np.empty((array.size, 8), dtype='S1') # <1 us
rgb = intensity_to_rgb(array, minval, maxval) # 200 us
rgbtohex(rgb, array.size, hex_) # 1300 us
return hex_.view('S8').astype('U8')[:, 0] # 1300 usThis is the other variant:
def intensity_to_hex2(array, minval=None, maxval=None):
import codecs
hex_ = np.zeros((array.size, 8), dtype='B') # 3 us
rgb = intensity_to_rgb(array, minval, maxval) # 200 us
hex_encoded = codecs.encode(rgb, 'hex') # 39 us
bytes_ = np.frombuffer(hex_encoded, 'B') # <1 us
bytes_2d = bytes_.reshape(-1, 8) # <1 us
hex_[:, 0] = ord('#') # 4 us
hex_[:, 1:7] = bytes_2d[:, 0:6] # 48 us
return hex_.view('S8').astype('U8')[:, 0] # 1300 usThere was a problem hiding this comment.
This is great!
Yeah, I couldn't understand how to do this efficiently in Python (I had to use a for loop), so I resorted to the C extension. I love your solution. Thanks!
|
I think this looks very nice. It seems there are quite some viewers popping up here and there these days, which I believe might be necessary, but I also believe viewers are often quite heavy in terms of LOCs and therefore they might be considered expensive in terms of maintenance. I believe ctapipe has 15k lines of code and with 1.7k lines, this PR adds a whopping 10% on top. I do not think that adding a lot of LOCs alone is a bad thing by itself, but let's assume for a moment that in 6 month from now we realize that of the 3 different existing viewers this particular one is not used by too many people... Then I personally would consider removing it maybe... or not if it turns out that is simply works without any maintenance. That being said, I have no real clue of Bokeh, but I always wanted to learn it :-D |
| elif v == 'peakpos': | ||
| peakpos = e.dl1.tel[t].peakpos | ||
| if peakpos is None: | ||
| self.image = np.zeros(self.image.shape) |
There was a problem hiding this comment.
For the other cases you say:
if samples is None:
self.image = None
but for peakpos, there is an exceptional handling of this case. Could you explain the difference?
|
I dream of a viewer, which does not need to be modified, when someone wants to plot a field from a Container, which was just added. I think, if there is a language which might be able to do this, then it is Python. I'm not sure how to do this, but as a first step I tried to refactor the case handling for the view_options out of the So def _set_image(self):
e = self.event
v = self.view
t = self.telid
c = self.channel
time = self.time
if not e:
self.event_viewer.log.warning("No event has been provided")
return
tels = list(e.r0.tels_with_data)
if t is None:
t = tels[0]
if t not in tels:
raise KeyError("Telescope {} has no data".format(t))
try:
self.image = self._view_options[v](e, t, c, time)
self.fig.title.text = '{0} (T = {1})'.format(v, time)
except:
self.image = NoneIf one would consider to combine the name and the action for each of the available view-options in a table ... maybe like this: class BokehEventViewerCamera(CameraDisplay):
def __init__(self, event_viewer, fig=None):
# [...]
self._view_options = {
'r0': lambda e, t, c, time: e.r0.tel[t].waveform[c, :, time],
'r1': lambda e, t, c, time: e.r1.tel[t].waveform[c, :, time],
'dl0': lambda e, t, c, time: e.dl0.tel[t].waveform[c, :, time],
'dl1': lambda e, t, c, time: e.dl1.tel[t].image[c, :],
'peakpos': lambda e, t, c, time: e.dl1.tel[t].peakpos[c, :],
'cleaned': lambda e, t, c, time: e.dl1.tel[t].cleaned[c, :, time],
}If we would go for this way of writing it down, I believe what we see is, that the places in code, we need to modify to allow for plotting of an additional field from the container was reduced. But we also lost at least one feature: non-specific title text:
|
|
This looks really nice. Good Job. I'm just wondering whether this might be better located in a standalone package outside of ctapipe? |
|
re @dneise - Such a viewer could be even more generic, since Containers can be looped over and accessed by field name string. One could provide a dropdown with the base container name, telescope name, and field name which would automatically be "r0, r1, dl0, dl1...", "1,2,3,4...", "image", "waveform", or even simply search recursively for all fields in any subcontainer that have the same geometry as the image, or something like that. Something to think about for a future feature. re where it should go: I'm also not sure about whether this should be in ctapipe, or wether there should have a separate "ctapipe-viewers" repo. It would take some time to set another repo up, but might make it more clean and easier to maintain. Its a similar question for the "flow" module, which is a bit out of date and could be easily separated. Perhaps for now, it's ok to have it in the main repo and we can separate things later? Another comment: I would suggest at the very least adding a top-level |
|
@dneise I like that "dream" too. I have incorporated the updates you have suggested, it should help make the utilisation of an automatically generated "view_options" easier a future update. |
that's great ... I hope so too |
Used bokeh.server.server.Server to run file_viewer through `python` instead of `bokeh serve` Created entry_point ctapipe-event-viewer Rearranged directory structure of tools/bokeh
|
I have included all the comments so far. The file viewer can now be ran with either: Codacy is complaining about jinja2.Environment, but I tried the change it suggests and it breaks bokeh entirely. If everyone is happy, I am ready for this PR to be merged. |
|
At the moment it seems to break when I feed a digicampipe fits file into it: For testing I use the Update: |
It appears you aren't filling the subarray container in sst1meventsource.py. So not a fault of the file viewer :) |
This answer makes me feel ridiculed and not taken seriously. I believe it was meant to be funny, but let me just tell you, it was not funny for me. Apart from that I would like to mention these points 1. Thank youYou found a problem in 2. Personification of code
I was calling: I assume by "you aren't filling" you refer to the 2. Depending on deprecated featuresI believe new code should try to not depend on deprecated features of the current code base. ctapipe/ctapipe/io/containers.py Lines 401 to 404 in c7d73cd If this viewer you are proposing to merge into the master does make use of deprecated features, then I believe it was good that we spotted it. 3. We should care about our usersSo ... do I understand correctly that you find it acceptable to merge this viewer into the master, knowing that any user who wants to try it on a digicam or nectarcam file will see the same exception as I reported? How do you suppose our response should be, if users write issues asking why they cannot use the nice bokeh viewer on their files? Answering "not a fault of the viewer" is in my understand not very helpful. |
|
I apologise, I wrote that comment in a rush before I caught a bus so that you weren't left hanging without a response (after downloading the file and the protozfits software to double check the problem). I was not aware this container is deprecated. To my understanding it is the only method available to obtain the pixel positions from the event container. Yes, I should refer to us all as owners of the code. However, I (and the majority of contributors) aren't familiar with the organisation of SST1M, and were unqualified to make the sst1meventsource, and are subsequently unqualified to figure out the best method to supply the pixel positions that accompany the data. In the TargetIOEventSource, we utilise the TargetCalib library which has knowledge of the pixel positions of our camera. Again, sorry for the careless response. |
|
Thank you for your kind understanding... |
Added bokeh>1.1 as a dependency to ensure compatibility
* master: (60 commits) Add test that shows slicing breaks cam geom and fix it (cta-observatory#782) fix ctapipe build failure (cta-observatory#811) fix package name for yaml (should be pyyaml) (cta-observatory#810) Implement number of islands (cta-observatory#801) fixed ranges of cam-display so they correspond to fixed toymodel sims (cta-observatory#808) Fix unknown section example warning (cta-observatory#800) Fix timing parameters for case when there are negative values in image (cta-observatory#804) Update Timing Parameters (cta-observatory#799) speed up unit tests that use test_event fixture (cta-observatory#798) Add unit to h_max in HillasReconstructor (cta-observatory#797) Codacy code style improvements (cta-observatory#796) Minor changes: mostly deprecationwarning fixes (cta-observatory#787) Array plotting (cta-observatory#784) added a config file for github change-drafter plugin (cta-observatory#795) Simple HESS adaptations (cta-observatory#794) add test for sliced geometries for hillas calculation (cta-observatory#781) Impact intersection (cta-observatory#778) updated main documentation page (cta-observatory#792) Implement concentration image features (cta-observatory#791) Fix bad builds by changing channel name (missing pyqt package) (cta-observatory#793) ... # Conflicts: # ctapipe/calib/camera/dl1.py
|
Can this PR be accepted yet? The only failures are those present in the master. |
|
yes, i think this is fine to merge once I get the tests working again. I'll do it shortly. |
Created an interactive event plotter that utilises the bokeh package (https://github.com/bokeh/bokeh)
This plotter shows both the camera and the waveform. It allows the selection of events, pixels, and time slices, and also allows the viewing and configuration of various calibration stages.
All EventSources are compatible with it, and all geometries are supported. However, all pixels are displayed as squares at the moment.
This tool requires a bokeh server to run:
bokeh serve ctapipe/tools/bokeh/file_viewerTo pass arguments to the tool:
bokeh serve ctapipe/tools/bokeh/file_viewer --args -f /Volumes/gct-jason/data/170330/onsky-mrk501/Run05477_r1.tio