Skip to content
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

EpochEncoder enhancements #16

Open
11 of 19 tasks
jpgill86 opened this issue Aug 25, 2018 · 7 comments
Open
11 of 19 tasks

EpochEncoder enhancements #16

jpgill86 opened this issue Aug 25, 2018 · 7 comments
Milestone

Comments

@jpgill86
Copy link
Collaborator

jpgill86 commented Aug 25, 2018

The EpochEncoder allows users to mark blocks of time with labels by creating epochs. In its current form, there can be only one label for any given time, i.e., labels are mutually exclusive. This is useful for marking periods that can be classified as belonging to only one state, e.g., a visual stimulus is one color or another.

However, the mutual exclusivity of labels limits the utility of the EpochEncoder in other situations. For example, it would be useful to be able to bracket the beginning and end of a repeated behavior as well as phases within each behavior, such as inhalation and exhalation in respiratory cycles, or stance and swing in gait cycles.

This can be accomplished now by creating separate EpochEncoders for each set of mutually exclusive states, such as a first EpochEncoder to mark each cycle of a repeated behavior, and a second EpochEncoder to mark phases within each behavior. However, this clunky workaround wastes a lot of screen real estate, since the EpochEncoder's controls and built in EpochViewer take up a lot of space.

I propose enhancing the EpochEncoder by allowing it to handle both mutually exclusive and non-mutually exclusive sets of labels, so that epochs could be created that overlap in time. I can think of a couple ways this could be implemented, and I'm interested in feedback on what seems best.

First, this check in WritableEpochSource that loaded epochs do not overlap would need to be removed.

Next, a method for controlling when mutual exclusivity should be enforced needs to be designed. A simple solution would be to implement something like a modifier key for controlling this behavior: When shortcut keys are used to assign labels, holding Shift toggles mutual exclusivity. For example, pressing a shortcut key without holding Shift would delete all existing epochs in the range when creating the new epoch (current behavior); pressing the shortcut key while holding Shift would create the new epoch without deleting existing epochs in the range. A checkbox in the global options could allow this logic to be reversed and to control the behavior of the Apply button.

A more complex approach would be to allow the user to create pre-defined sets of mutually exclusive labels. For example, a first set of labels could contain "Behavior Type 1" and "Behavior Type 2"; a second set of labels could contain "Behavior Type 1 Phase 1", "Behavior Type 1 Phase 2", etc. Users could specify these sets by passing a 2D list of lists of strings to the possible_labels parameter, rather than a 1D list of strings. Additional enhancements to the GUI could allow users to reorganize these sets on the fly. (EDIT: After implementing the simpler solution of toggling mutual exclusivity with Shift or a button and finding that it works well, I think this more complex solution feels like overkill. It's unlikely that I will try implementing it soon, but I'll keep it on the list.)

These two approaches could be combined for maximum flexibility.

If overlapping epochs are allowed, the method of implementation could affect how the Fill Blank feature must function. If pre-defined sets of mutually exclusive labels are implemented and provided by the user, then the Fill Blank functionality could be applied to each set separately in the same way it currently functions. If only a modifier key functionality is implemented, Fill Blanks may need to be limited to filling just those periods of time where no epochs are defined (i.e., the true blanks).

The "flat" view mode would not be available for labels that can overlap, or would need to work differently.

For use cases such as marking phases of a repeated behavior, it would be helpful if the EpochEncoder allowed for epochs to be cloned with a new label, and for epochs to be divided into multiple new epochs. For example, if one respiratory cycle has already been marked in the EpochEncoder, its inhalation and exhalation phases could be easily marked by cloning the whole-cycle epoch and dividing it at the transition point between phases. This ensures that the boundaries of the phases and the whole cycle are aligned.

Relabeling and deleting individual epochs using the EpochEncoder would also be very useful. Finding particular epochs in the data table could be made easier if clicking on their rectangles in the plot automatically selected the corresponding entry in the table.

I think that most of the options currently exposed in the Global Options panel are changed rarely enough that the whole panel could be hidden and accessed instead by either double-clicking on the epoch viewer or by pressing the "Colors and keys" button (which could be renamed to "Options"). This would free up a lot of space on the screen. The exception is the new_epoch_step parameter; some users may change this value frequently, so it should be exposed on the main control panel.

Removal of the central panel could free up space and allow the control panel currently on the left to be reorganized, or the data table could be made larger. I don't have specific plans for this yet.

It also seems more natural to me to use the number keys as default shortcut keys for assigning labels, rather than an arbitrary selection of letter keys. I propose changing these.

To summarize, here is a list of features I'd like to add to EpochEncoder (EDIT: Updated based on conversation below and as features are implemented):

  • Fix numerical precision of epoch start and stop times
  • Assign numbers as default shortcut keys
  • Remove central Global Options panel and combine with "Colors and keys" popup
  • Remove assertion that epochs loaded from WritableEpochSource do not overlap
  • Add modifier key and global option for toggling exclusion of existing epochs when creating a new epoch
  • Add controls for epoch insertion mode (mutually exclusive or overlapping) to left panel
  • Add click on rectangle to select corresponding entry in data table
  • Add mechanism for relabeling existing epochs
  • Add mechanism for deleting individual epochs
  • Add mechanism for cloning epochs with new labels
  • Add mechanism for dividing epochs
  • Add controller for new_epoch_step parameter to left panel
  • Allow pre-defined sets of mutually exclusive labels
  • Allow on-the-fly reconfiguration of sets of mutually exclusive labels
  • Update Fill Blanks functionality for scenarios with overlapping epochs
  • Fix "flat" view mode when epochs overlap
  • Check that seek performance with lots of epochs is not too poor
  • Add assert_epoch_not_overlap_at_load
  • Add split at mouse cursor

I'm eager to hear what others think of these proposed changes, and if they have any suggestions for improvements.

@jpgill86
Copy link
Collaborator Author

jpgill86 commented Aug 26, 2018

Here's another idea.

Due to quirks of floating point arithmetic, epoch start and stop times / durations sometimes have odd boundary times, like 0.30000000000000004.

For example with new_epoch_step set to 0.1, creating 4 different epochs in a row will result in these representations in the data table (bottom-right panel in ephyviewer) and saved CSV file (right side of screenshot),

epochs-1

Not only is this unattractive and inconsistent, but it can make managing epochs difficult.

Sometimes undesirable results will occur when the user attempts to overwrite one label with another. For example, after creating the four epochs shown above, if the user clicks on row 3 in the data table to jump to that time and presses the shortcut key for the first label twice to change the labels of the 3rd and 4th epochs, the EpochEncoder will fail to properly align boundaries, and a tiny sliver of the old 4th epoch will remain.

epochs-2

I think these undesirable effects could be avoided by discretizing time (perhaps with microsecond resolution, since it's unlikely a user would need to block out time with more precision than that), or otherwise by adjusting NumPy settings. I don't have much experience with managing numerical precision in Python, so I'm not sure what's the simplest/best solution, but I am motivated to look into it.

EDIT: I solved these problems in e257e14 using some simple rounding and thresholding.

@jpgill86
Copy link
Collaborator Author

In pull request #13, I've begun implementing many of these ideas. I'll check them off the list as I go.

@jpgill86
Copy link
Collaborator Author

jpgill86 commented Sep 7, 2018

EDIT: I moved this post to here from PR #13 since it requires discussion and a lot of work.

WritableEpochSource is currently permitted to have only 1 channel (assert chan==0 in get_chunk*). That channel has a name, but it's not used for anything by the EpochEncoder. Instead, the label array tracks epoch types. When plotted by the EpochEncoder, epochs are grouped by their label and plotted in separate rows. The WritableEpochSource's list of possible_labels is compared against the label array, not the channel name.

This is rather different from how multi-channel epoch sources work with the EpochViewer. Each channel in an InMemoryEpochSource has its own name which is used by the EpochViewer to group the epochs for plotting in separate rows. The Epoch_Viewer's display_labels option actually relies on the channel name, not the label array. The label array is ignored by the EpochViewer, but if the InMemoryEpochSource is used with the EventList, the label array is displayed as additional information.

To be concrete, here are examples of how the data is represented differently by the two epoch sources.

For InMemoryEpochSource, data is organized like this for use with EpochViewer, with any number of channels (here 2):

source.all = [
    {
        'name': 'Channel 0',             # 'name' used for labeling rows in EpochViewer
        'time': array([ 0.5,  1.0]),
        'duration': array([ 0.1,  0.1]),
        'label': ['Label 0', 'Label 1']  # 'label' not used by EpochViewer, but is used by EventList
    },
    {
        'name': 'Channel 1',             # 'name' used for labeling rows in EpochViewer
        'time': array([ 1.5,  2.0]),
        'duration': array([ 0.1,  0.1]),
        'label': ['Label 0', 'Label 1']  # 'label' not used by EpochViewer, but is used by EventList
    }
]

For WritableEpochSource, data is organized like this for use with EpochEncoder, with exactly 1 channel:

source.all = [
    {
        'name': '',                      # 'name' not used by EpochEncoder
        'time': array([ 0.5,  1.0]),
        'duration': array([ 0.1,  0.1]),
        'label': ['Label 0', 'Label 1']  # 'label' used for labeling rows in EpochEncoder
    }
]

To the user, most of these differences aren't really important, but these inconsistencies mean that the data is accessed differently by the EpochEncoder and EpochViewer, with the consequence that if WritableEpochSource is used with EpochViewer, all epochs are plotted in a single row with no label.

I propose changing WritableEpochSource to be more like InMemoryEpochSource in terms of data representation by grouping epochs of different types in different channels, rather than using the label array to distinguish types. This will create a uniform data representation and improve compatibility between different epoch sources and viewers, allowing WritableEpochSource to be plotted correctly with EpochViewer, and decreasing the work needed to convert an InMemoryEpochSource to a WritableEpochSource (something I may propose doing soon). The label array would, for now, not be used by the EpochEncoder, but it could eventually be used for providing extra commentary for each epoch (something else I might propose).

EDIT: Instead of beginning work on this right away, I'd like to get confirmation that this is a reasonable direction to take things. I'd also like to see PR #13 merged before I make any additional major changes, since the changes proposed in this post will reorganize a lot of things, making PR #13 harder to review.

@samuelgarcia
Copy link
Collaborator

To clarify a bit.
I my mind each channel deal with different kind f things.

source.all = [
    {
        'name': 'Sleep state',
        'time': array([ 0.5,  1.0]),
        'duration': array([ 0.1,  0.1]),
        'label': ['Awake', 'SWS'],
    },
    {
        'name': 'Weather'
        'time': array([ 1.5,  2.0]),
        'duration': array([ 0.1,  0.1]),
        'label': ['Sunny', 'Warm'],
    }
]

So yes for this feature.
Taking one channel was not a choice but a limitation by the complexity of the GUI.

In my mind EpochViewer for multi channel stuff should also inclued colors by channel for the labels sets.

@jpgill86
Copy link
Collaborator Author

@samuelgarcia, that's a use-case I hadn't considered, but I like it a lot! This is very close to my original post's suggestion of "pre-defined sets of mutually exclusive labels", and your example makes me much more enthusiastic about implementing it. I could imagine reworking possible_labels, changing it from a flat list to a dictionary that specifies channel names and the labels available within each:

possible_labels = {
    'Sleep state': ['Awake', 'SWS'],
    'Weather':     ['Sunny', 'Warm'],
}

Each label would have it's own color and shortcut key (1 = "Awake", 2 = "SWS", 3 = "Sunny", 4 = "Warm"). "Sleep state" epochs would be plotted in one row, and "Weather" epochs would be plotted in a second. Within a single channel, epochs could not overlap, but across channels they could. This obviates the need for a confusing exclusive_mode parameter, a Shift modifier key, and slower algorithms for get_chunk*, as we've discussed in #13.

I think this could make EpochEncoder very intuitive, and in hindsight it seems so much simpler than my approach in #13, haha. Do you agree that this behavior for EpochEncoder would be better and less confusing?

@samuelgarcia
Copy link
Collaborator

Yep.
I agree.
Thank you very much for taking the big work on your back.

@jpgill86
Copy link
Collaborator Author

jpgill86 commented Oct 1, 2018

OK, I'll need some time to work on this. Perhaps I'll abandon #13 and start a new pull request since this is a very different (but much better) direction. Not sure yet.

@jpgill86 jpgill86 added this to the Future milestone Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants