add single wf visualiser - #85
Conversation
There was a problem hiding this comment.
First comments from a user perspective before I dive into the code a bit more closely:
- The example config (
packs/config/vis_wd1_1channel.conf) should point by default at one of the data files inpacks/tests/data/. - Due your sampling rate, using
plt.plot(..., marker = 'o')makes waveforms with larger sampling size look like so:
So changing it to marker = 'o-' would resolve this
- The visualiser works on 'waveform number', but for files with multiple channels (WD2), how does this correlate? The visualiser should relate channels and events to the relevant waveform for this case. For one-channel cases, it can just say 'channel 0' :)
|
For dynamically using mule_dir in the config so that the example config can be run from anywhere, I think I would need to change io.read_config_file() as configparser does not support this natively. The way I've left it so far it has to be run from mule/${any_subfolder}. |
jwaiton
left a comment
jwaiton
left a comment
There was a problem hiding this comment.
Looks good and works as intended! A couple of things I'd like changed before it can be approved.
- General documentation of the usage of
tkintercould be improved. It's not been used anywhere else in MULE, so it would be nice if there is some explanation of how it's working (filling and padding seems relatively abstract until you read the documentation). - Minor style changes.
| ax.plot(time, single_wf, | ||
| marker='o-', markerfacecolor='None', linestyle='None', markersize=1) |
There was a problem hiding this comment.
| ax.plot(time, single_wf, | |
| marker='o-', markerfacecolor='None', linestyle='None', markersize=1) | |
| ax.plot(time, single_wf, | |
| marker='o', markerfacecolor='None', linestyle='solid', markersize=1) |
The marker style o- doesn't seem to work for me, this resolves it and adds a solid line to allow for a better visualisation of more discrete points in time.
Before:
After:
| case other: | ||
| raise RuntimeError(f"process {other} not currently implemented.") | ||
| except KeyError as e: | ||
| print(f"\nError in the configuration file, incorrect or missing argument: {e} \n") |
There was a problem hiding this comment.
Are these cases (RunTimeError & KeyError) covered by a test?
I wrote some general ones for this sort of behaviour, but please double check :)
| traceback.print_exc() | ||
| sys.exit(2) No newline at end of file |
There was a problem hiding this comment.
We can do something nicer with the handing, as they're duplicated across all differing packs. Might be an interesting job for @Tedsmith100 👀
| from packs.core.io import read_config_file | ||
| from packs.core.core_utils import check_test | ||
| from packs.vis.visualise_utils import visualise_waveform |
There was a problem hiding this comment.
Super nitpicky, but try and align imports :)
| import numpy as np | ||
| import pandas as pd | ||
| import matplotlib.pyplot as plt | ||
|
|
||
| from matplotlib.backends.backend_tkagg import FigureCanvasTkAgg | ||
| import tkinter as tk | ||
| from tkinter import ttk | ||
|
|
||
| from packs.core.io import load_evt_info, load_rwf_info | ||
| from packs.proc.calibration_utils import subtract_baseline, collect_sidebands |
There was a problem hiding this comment.
Likewise try to align imports.
I find the inclusion of tk as shorthand for tkinter and the separate import of ttk a bit messy. I'm unsure of a nicer method right now other than using tk.ttk.___ every time you want to use that part of the library, which I think is also a bit ugly. Up to you on this front.
|
|
||
| # controls frame | ||
| ctrl = ttk.Frame(root, padding=8) | ||
| ctrl.pack(fill=tk.X) |
There was a problem hiding this comment.
Likewise, it would be worth commenting here on what this is doing, its taking control of the root frame and adding padding below it for a controller section.
| entry.bind("<Return>", on_entry) | ||
| entry.bind("<FocusOut>", on_entry) |
There was a problem hiding this comment.
Likewise, I'd explain what the controls do here. Allows for adjustment of waveform via the editable number in the GUI
| try: | ||
| val = int(entry_var.get()) | ||
| val = max(0, min(val, max_wf)) | ||
| entry_var.set(val) | ||
| slider_var.set(val) | ||
| plot_waveform(val) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
Pretty nice bit of code, I'd try and write a test that forces the ValueError, and checks that val does get set to 0 and the max_wf value if you give it some weird inputs.
For example, currently inputting strings doesn't do anything which isn't bad behaviour at all, but you may want to sanitise the inputs so people can't do anything malicious/stupid.
| pass | ||
|
|
||
| def on_slider(value : str | None = None): | ||
| """Command for ttk.Scale. Sync the entry box to the slider position and redraw the selected waveform.""" |
There was a problem hiding this comment.
likewise, make the comment a match the formatting :)
| ttk.Label(ctrl, text=f"(0 – {max_wf})").pack(side=tk.LEFT) | ||
|
|
||
| # draw initial waveform | ||
| plot_waveform(0) |
There was a problem hiding this comment.
Should probably be a try/except here. I'd create a test for an incorrect file input (you want a specific error to be returned), or a h5 file with the correct path/group/node but the input doesn't work as expected.
2 participants
Adds to
packsthe choice ofvis. This allows the mule hdf5's to be visualised one waveform at the time.Currently, the only option is
waveformas in single waveform visualiser. Other options such as average, smoothened single waveform, and FFT will be added later on.This initiates a GUI with a scrollable element that allows for the waveforms in the mule hdf5 to be visualised one by one.