Markers and nonlinear timescale

[philipstarkey]

Original report (archived issue) by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

The original report had attachments: runviewer.png

---

In our group we extended labscript to allow for adding markers in the experiment script to make shot analysis in runviewer easier. These markers are then be displayed in runviewer to give a quick overview of the experiment stages without needing to know the exact times of where things happen. Additionally we adjusted the time axis to make the markers space evenly, since there are longlasting stages in wich not much is happening (loading the MOT) and stages where a lot is happening on a small scale. This also allows for a quicker overview without needing lots of zooming in. To make things a bit clearer I attached a screenshot of a shot in our runviewer.

The drawback of this change is that there (at least currently) is no way of displaying multiple shots at one, since they might differ in their markers. This is why I raise the question: is this something that would be of use for the main branch(in wich case I would create pull requests for this change) or is the drawback too great?

Edit: There also is a tooltip displaying the current time the mouse is hovering over so thats why our time axis is empty

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

• Edited issue description

[philipstarkey]

Original comment by Chris Billington (Bitbucket: cbillington, GitHub: chrisjbillington).

---

This is great, and a feature I've had in mind for some time! It's extremely useful. The screenshot looks really nice!

I'd be eager to look at the code. I assume this involves changes to both labscript (adding a way to mark times) as well as runviewer? Every lab I've seen so far using labscript has some kind of function decorators or printlines or otherwise some way of splitting their experiment into stages saying at what point in time each starts and ends (also useful for debugging cryptic message from labscript telling you at what time you did something wrong with little other info).

So yes, that would be useful to include. Personally I would vote on giving up showing multiple shots in favour of this, if that's what it would take. We could modify runviewer to allow showing different shots in different tabs or something, even though it wouldn't make sense to show them side-by-side.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

The code needs a bit of cleaning up and a few changes which I'll be doing in the next few days before writing a pull request. It is still possible to select multiple files and the variable self.selected_shot is not being use and assigned in a good way. A feature I also forgot to mention is that the Shutter Outputs display also have markers that display the actual open and close time.

But to get a feel for the changes you can take a look at my code but as I said this is still work in progress

As for the changes to labscript.py

#!python

def add_marker(t, label, color=(0,0,0)):
    #color in rgb
    markers[t] = {"label":label, "color":color}

def save_markers(hdf5_file):
    markers_dataset = hdf5_file['runviewer'].create_dataset('markers', shape=(1,))
    for i in markers:
        markers_dataset.attrs[str(i)] = str(markers[i])

def save_shutter_times(hdf5_file):
    shutter_times_dataset = hdf5_file['runviewer'].create_dataset('shutter_times', shape=(1,))
    for i in shutter_times:
        shutter_times_dataset.attrs[str(i)] = str(shutter_times[i])

Addition to labscript_init()

#!python

#markers and shutter times#
    markers = {}
    _builtins_dict["markers"] = markers
    shutter_times = {}
    _builtins_dict["shutter_times"] = shutter_times

Addition to Shutter.open()

#!python

        # save the actual open time (when the shutter should be opened)
        if self.name in shutter_times:
            shutter_times[self.name][t] = 1
        else:
            shutter_times[self.name] = {t:1}

Addition to Shutter.close()

#!python

        # save the actual close time (when the shutter should be closed)
        if self.name in shutter_times:
            shutter_times[self.name][t] = 0
        else:
            shutter_times[self.name] = {t:0}

Generate Code:

#!python

        save_markers(hdf5_file)
        save_shutter_times(hdf5_file)

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

I do like the idea of markers (it's been something we've talked about since the early development stages I believe).

I'm not all that happy about losing the ability to compare two shots to each other. The non-linear time would also make it difficult I think to see how long things were if you didn't have a marker delimiting that feature, which I also think is not great. However, I can see the benefit of being able to view it like you propose, but I don't think that should be the only way of viewing it. I'm not sure what the best approach is here, but I'd like to see the interface not lose functionality.

With regards to shutter open/close markers, yes this would be good to add in too! It does need to be done in some sort of modular way though - as in runviewer needs support for handling arbitrary subclasses of DigitalOut and AnalogOut and DDS. Currently I don't think we do this at all, we just ask the runviewer class for each device to return a trace. This mode of operation good, because we get a faithful representation of what the hardware is actually doing based on the hardware instructions in the HDF5 file (this is the primary aim of runviewer). I guess what we need is for runviewer to look at the class of each channel attached to a device and call some API to get additional information about the channel (like markers for shutters).

I'll also note that I don't think you need to save additional information in the HDF5 file about the shutter times. The shutter delays are already stored in hdf5_file['/calibrations/Shutter/'] which you can use to back calculate the locations of the markers from the existing runviewer trace. We probably need to store the Shutter.open_state parameter, but that should probably go in the connection table using the set_passed_properties decorator anyway (it is after all, a hardware configuration parameter which could be changed and thus should prevent previously compiled shots from running).

With regards to making the pull request, we haven't (yet) developed guidelines for making pull requests. However, we're moving towards doing work in a feature branch of our own repository, and then when that feature is done, making a pull request from that. It is thus good to try and separate out new work into separate branches if they are unrelated. Similarly, if one feature builds on another, you can branch from your branch once you make the pull request for the first feature. I mention this because I had a look at your repository and it looks like you've made a bunch of formatting changes (probably PEP8 styling?). This is great! (we should follow PEP8) but if you make a single pull request with the styling changes and the new marker features, it's going to be much harder for us to evaluate your code! It would be much easier if this was split into 2 pull requests so we can see which changed lines relate to which feature :) You might consider splitting the shutter markers off into a third pull request, and maybe the rescaling of the time axis into a 4th pull request. This gives us the option of not including your rescaled time axis if we can't agree on a good interface design, while still giving everyone the ability to still add markers (which I think is a really great feature we should include).

Anyway, got a bit off topic here. I'm happy to discuss more the possible UI designs to both maintain the current feature set (where you can compare traces) and a rescaled time (based on marker location) feature! I'm also happy to talk more about what our best practice for making pull requests should be (as I'm sure @chrisjbillington is too).

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

@philipstarkey thanks for the very comprehensive answer.

The nonlinear timeaxis is the reason for posting the question here. If you have any ideas on how compare functionality can be maintained and the nonlinear time can be achieved I'd love to hear it. But to be honest I don't have any idea how this can be done in a good way.

The reasoning behind writing additional information for the shutter times into the h5 file is just that is was easy and didn't need checks for being a Shutter or not. Also the overhead is minimal.

Yes it's PEP8 and sorry for that. My editor converts everything to PEP8 upon saving. I didn't think of that. I'm new to bitbucket and haven't quiet figured things out. I'll create a PEP8 pull request right away.

Ok so my plan is:

1. PEP8 pullrequest for runviewer

2. Un related pull request to make the current horizontal splitter a vertical one and the vertical one a horizontal to allow for a longer plot x range by default

3. Pull request for the markers in labscript

4. Pull request for the markers in runviewer (even tough it looks quite ugly without Nonlinear Time)

5. Pull request for the shutters in runviewer (and in labscript if I decide to keep it the way it is)

6. Nonlinear Time Pullrequest

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

What about a drop down menu (say next to the reset axes buttons) that allows you to select which shot will be linearly spaced based on the markers. Possibly you'd need to hide the markers for the other shots (but there could be a check box next to each shot in the list of shots that says whether to display the markers for that shot, or something along those lines). This idea needs fleshing out some more, but could be a possibility?

That makes sense with regards to the shutter times. The very first, original version of runviewer, did things like this (although not that exact feature). However, when I rewrote it, I deliberately made the decision to back calculate what was happening from the hardware instructions. This increase the chance that a bug in the code that writes the hardware instructions will be found, since they directly effect what is shown in runviewer. As such, I think it would be preferable to maintain this format, rather than saving additional information that only faithfully represents what is happening if there are no bugs.

That said, I'm happy to have my mind changed about this. I kind of made the decision by myself at the time, so there has not really been a discussion on the merits of this approach with anyone!

Your plan for pull requests looks good! Thanks for putting in the time to contribute back to the main repositories!

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

I'm having trouble wrapping my head around your proposal. A dropdown would allow for selecting what shot so scale to yes. We could also remove the marker lines in the plots themselves and just keep the top marker plot with the labels. But I think this would just cause confusion and not really help in any way. But since each channel has one plot and not each shots's channel things would look really strange when things are only a tiny bit shifted between shots. For example if there is a ramp that is half way in a marker. Or there is a delay added in one of the shots that is greater then one of the markers spacings things would be very awkwardly spaced. What happens if you tick 2 Shots from different scripts? I dont think that a non linear timescale and comparability can be achieved in a easy way.

One way that things could work out is that we display each shots channel in a separate plot and create a smart way of linking the differently scaled timeaxes(each with its own markers). But this is hard to get right and I don't feel like this is something I can do.

As I said I'll give rewriting shutter times a try. And if things work out I'm happy removing the current way.

I'm happy to! It's in my groups own interest to come closer to the main branch to make updates easier. This also helps eliminate bugs :)

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

Yeah, you're right, my idea doesn't work. I'll keep thinking about it. We may just have to resort to some sort of toggle that changes between shot comparison and the ability to swap to this new mode you're proposing.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Ok so I created the first runviewer pull request.

I would now like to tackle the Shutter feature before getting into discussions about the nonlinear time. @philipstarkey as you already mentioned I would need to somehow figure out what the openstate of a Shutter is. I would as you said add something like this to labscript.py:

#!python

class Shutter(DigitalOut):
    description = 'shutter'

    @set_passed_properties(
        property_names = {"connection_table_properties": ["open_state"]}
        )
    def __init__(self,name,parent_device,connection,delay=(0,0),open_state=1,
                 **kwargs):

Before I propose this change in labscript: you mentioned something about this preventing previously compiled shots from running, how so?

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

@PhyNerd So you need the open_state parameter to be saved in the HDF5 file in order to access it from runviewer.

There are 2 places to save it, device_properties or connection_table_properties. Anything in connection_table_properties will be used to make a comparison with the BLACS connection table when shots are submitted. Shots that don't match will be rejected from running. I think it should go in the connection table properties, because the connection table should represent the hardware, and a change of hardware state (such as inverting a shutter) needs to be represented in the connection table (the fact that we don't save it already is an oversight). You don't want to re-run an old shot if the Shutter has been inverted in the meantime!

So it's not a big deal, it just means you need to recompile your BLACS connection table and you can't re-run old shots. Just something to be aware of.

It might be worth opening a new issue specifically for the shutter label feature, so we can discuss the runviewer implementation details further before you start writing it. For instance, I think it would be good to define what the runviewer Shutter implementation will be allowed to do (can it change the digital trace, or just add additional information to the plot? What additional information should we allow and what data structure/format should we store it in? that sort of thing!)

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Ok how about we make nonlinear time a feature that is active when viewing one shot only and disable it when more shots are viewed? This could cause a bit of confusion as the time axis would change upon selecting a second file, but I can't really think of a better solution. Would this solution be suitable @philipstarkey?

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

Yeah, that's probably the best that can be done.

I think probably the interface should contain a button to swap between nonlinear and linear time axes (next to the rescale y-t axes buttons). This button should be disabled when more than one shot is selected and it's state ignored. If only one shot is selected, the state of the button determines the scaling of the time axes.

[philipstarkey]

Original comment by Shaun Johnstone (Bitbucket: shjohnst, GitHub: shjohnst).

---

Hi Jan, I've just checked out your nonlinear time code (running the tip of the APQ branch, so a little out of date compared with the markers branch, but it seems to run ok). I assume that you're waiting for the markers pull request to go through before you make a request for the nonlinear code? I've had a couple of thoughts on possible further features that could be added, and figured that given the basic functionality hasn't been merged in yet this would be the best place to discuss them.

It would be very cool if instead of just plotting the markers spaced evenly along the time axis, you could instead click and drag the marker lines to any location between the neighboring markers along the x-axis (you could add some sort of extra widget across the top of the plots where the clicking and dragging happens if it's easier). In this case, you'd probably want to disable scrolling and zooming in the time axis, as instead one would drag markers around to get the desired view.

I don't know how feasible this would be, but it could also be useful to be able to add in temporary markers (i.e. not actually saving them to the H5 file) by hand in runviewer. This way, if you wanted to zoom in on a particular region using the ideas above, you could click to add extra markers either side of it and drag them out.

In terms of the user interface, I imagine the above ideas working in a similar way to the gradient tool in Illustrator or Photoshop, where extra colour markers can be added along an axis, then moved along it to change the local rate at which the colours change (in our case, it would instead change the local rate at which time progresses along the axis). As to the actual implementation, it seems like it should be possible to have it re-scale in real time, as the bars are dragged, given we already have the plots re-sampling when zooming. It would require a new scaling function that took the relative distance of each marker along the axis as its input.

Let me know what you think, and whether any of these ideas are even possible!

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

Yes I'm waiting on the marker feature to finally merge. I didn't think it would take almost a year when I created the marker pull request. The APQ branch is quite outdated our lab uses GitHub and Git to manage our code. Therefore I usually don't update stuff here that is not related to pull requests.

Mh the dragging feature and the temporary markers definitely are within the realm of possibility. I think pyqtgraphs InfiniteLines support dragging any have a signal attached to it. In both cases we would need to rethink the current rescale implementation which was designed to be static. We could maybe even include it in the resample to make it faster.

But for the start we should first stick to the currently proposed simple feature wich we can than extend over time.

[philipstarkey]

Original comment by Philip Starkey (Bitbucket: pstarkey, GitHub: philipstarkey).

---

Yeah, sorry about the delays with merging things. Unfortunately with all of the main developers busy writing theses, it's been hard to find time to deal with the more complicated and/or major changes! We're all getting close to the end though, so I think things will pick up over the next few months!

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

@shjohnst I built very basic demo of the changes you proposed: https://pastebin.com/C2FHLQaT

I'm not really convinced anymore that this is a useful feature. The dragging of markers feels really clunky as one needs to move the neighboring markers out of the way which is just annoying. Scrolling is much more efficient for this. Same goes for the additional markers they just feel clunky and I haven't found a easy way to remove them after they were added.

[philipstarkey]

Original comment by Shaun Johnstone (Bitbucket: shjohnst, GitHub: shjohnst).

---

Thanks for looking into it! I think you're right that it may not be particularly useful unless we come up with an ideal interface for it. Let's leave it for now.

[philipstarkey]

Original comment by Jan Werkmann (Bitbucket: PhyNerd, GitHub: PhyNerd).

---

• changed state from "new" to "resolved"

Feature was added in d9e60de22733aa728cbf85dd5b4280c0801e4f00

[philipstarkey]

Original comment by Shaun Johnstone (Bitbucket: shjohnst, GitHub: shjohnst).

---

Actually, I think I've come up with an intuitive way of doing rescaling, since this issue has been resolved, and it's really a new feature request, I'll open a new issue about it.