GH-1404: Handle monitor delay errors #1438
Conversation
kschelonka
force-pushed
the
GH-1404/monitor-delay-error
branch
from
e788176 to
6d9ffc2
Compare
Codecov Report
@@ Coverage Diff @@
## rc/1.6.0 #1438 +/- ##
============================================
- Coverage 34.71% 34.59% -0.12%
============================================
Files 342 342
Lines 33264 33232 -32
============================================
- Hits 11547 11498 -49
- Misses 21717 21734 +17
Continue to review full report at Codecov.
|
kschelonka
force-pushed
the
GH-1404/monitor-delay-error
branch
2 times, most recently
from
72c0cdf to
5558d34
Compare
kschelonka
requested review from
pickles-bread-and-butter,
MatthewAitken,
dougollerenshaw and
djkapner
kschelonka
force-pushed
the
GH-1404/monitor-delay-error
branch
from
5558d34 to
941df71
Compare
There was a problem hiding this comment.
This looks great! It's reassuring to see that the calculated value is so consistent on the sessions you've tested against. Would it also be possible to run this on more than the 138 sessions mentioned above? Doing so might help expose more edge cases where the algorithm breaks down. It would also be great to see if the calculated value varies across rigs.
| LONG_STIM_THRESHOLD = 0.2 # seconds | ||
| ASSUMED_DELAY = 0.0215 # seconds | ||
| MAX_MONITOR_DELAY = 0.07 # seconds | ||
| REG_PHOTODIODE_INTERVAL = 1.0 # seconds |
There was a problem hiding this comment.
This value is determined by a variable that is set by the stimulus program (camstim). The variable determines how many monitor frames between each sync square state change (which the photodiode is measuring) and its default value is 60 (which leads to a 1.0 second photodiode interval at a monitor refresh rate of 60 Hz). If that value were to change, the expected REG_PHOTODIODE_INTERVAL should also change.
Accessing that variable would require coordination with @rhytnen to determine if/where it is logged.
As an alternative, I'd suggest a comment here to explain why this value is 1.0, which might aid future troubleshooting efforts if the default value in camstim were to change.
There was a problem hiding this comment.
Thanks, I tried to see if I could figure out the frame rate from the data but I didn't find any fields. For now I'll add a comment as suggested, but I can add a story to work with Ross/MPE to figure out if there is a way to get this dynamically
There was a problem hiding this comment.
Would it also be possible to run this on more than the 138 sessions mentioned above? Doing so might help expose more edge cases where the algorithm breaks down
Yes, I can run it for other sessions (though we will not have comparison results)
There was a problem hiding this comment.
@dougollerenshaw Did a random sample of 1000 out of all the ophys experiments we have in our well known files. Quite a few did not work properly because the sync file was not in the location the input_json expected, but I did get 721 results.
Here's the data by equipment type:
|
I'd suggest adding @rhytnen as a reviewer as well. |
|
could you please describe in |
kschelonka
force-pushed
the
GH-1404/monitor-delay-error
branch
2 times, most recently
from
51d3c41 to
05d5f02
Compare
kschelonka
force-pushed
the
GH-1404/monitor-delay-error
branch
from
05d5f02 to
90d8f94
Compare
In order to compute monitor delay, a sync square monitored
by a photodiode changes state every 60 frames ('sentinel
event'). Each transition from a rise/fall to a fall/rise
('photodiode event') represents a change in the state of
the sync square.
Previously only consecutive falling edges of the photodiode
were used to determine whether the photodiode events triggered
by sentinel frames had started. Only a falling edge was allowed
to be the final event in a photodiode stream, even though it
is possible that the last valid event could be a rising edge.
This led sometimes to missing valid events, and resulted
in an error (because the number of sentinel frames should match
the number of photodiode events).
The algorithm has been updated to examine consecutive events
instead of relying on a specific edge type. A transition marks
a sentinel frame if the next photodiode transition is roughly
1.0s after the previous or 1.0s before the next. Due to noisy
data we can't just look for all contiguous events of this type.
Instad we mark the beginning and end of the photodiode event
stream as follows:
1. The first valid photodiode event is the first event where
the time between the next two consecutive events are ~1.0s
2. The last valid photodiode event is the final event where
the last event where the time between the previous two
consecutive events are ~1.0s
Update the logging to show the min, max, and std delay.
No longer use a default value, but throw an error if there are
data issues or the value computed is outside expected range.
kschelonka
force-pushed
the
GH-1404/monitor-delay-error
branch
from
90d8f94 to
bd1d186
Compare
The get_keys method in time_sync.py was providing the wrong line labels to the classes using the method. It was naively assuming that keys existed in one set or the other with a clean division. This was not the case and the issue was unveiled when error handling was removed from monitor delay in #1438. The method now searches for the possible key values and if they do not exist assumes an old file where not all keys are present. In the case of key not present it defaults to using the oldest key denoted in VERSION_1_KEYS. This fixes the failing bamboo tests and is a more robust search for the future. Resolves: #1513 See also: #1438, #1519, #1451, #1507
The get_keys method in time_sync.py was providing the wrong line labels to the classes using the method. It was naively assuming that keys existed in one set or the other with a clean division. This was not the case and the issue was unveiled when error handling was removed from monitor delay in #1438. The method now searches for the possible key values and if they do not exist assumes an old file where not all keys are present. In the case of key not present it defaults to using the oldest key denoted in VERSION_1_KEYS. This fixes the failing bamboo tests and is a more robust search for the future. Resolves: #1513 See also: #1438, #1519, #1451, #1507
The get_keys method in time_sync.py was providing the wrong line labels to the classes using the method. It was naively assuming that keys existed in one set or the other with a clean division. This was not the case and the issue was unveiled when error handling was removed from monitor delay in #1438. The method now searches for the possible key values and if they do not exist assumes an old file where not all keys are present. In the case of key not present it defaults to using the oldest key denoted in VERSION_1_KEYS. This fixes the failing bamboo tests and is a more robust search for the future. Resolves: #1513 See also: #1438, #1519, #1451, #1507
Updated algorithm from from @dougollerenshaw:
change in the state of the sync square. Call these ‘photodiode events’
frames (where N = 60 by default). Let’s call these ‘sentinel frames’
transition is roughly 1.0s after the previous OR 1.0s before the
next (for N = 60 frames at a 60 Hz display rate). Call these ‘valid transitions’
We can't just use contiguous events because of noisy data, so like before we're looking for the start and end events. The main difference is that we allow the stream to start/end on either kind of edge (rising/falling). There was an issue with the last (valid) edge getting cut off when it ended on a rising edge, resulting in an error that set the delay to default.
In addition to updating the unit tests, I've also run regression testing for the 138 experiments for which we have the computed monitor delay output.
Out of 138 experiments, there were 18 that had the default value for the delay=
0.0351. Of the remaining 120, the monitor delay was computed exactly the same for 116.For the 4 experiments that returned different results, all were negligible differences:
The 18 experiments that previously returned the default value (due to code error) were computed with values in the expected range:
Note that the job will no longer return a default value for the monitor delay. If a data issue is encountered it will throw an error instead.