Investigate why monitor delay gets set to default value in OPHYS_TIME_SYNC_QUEUE #1404
Comments
|
Of the 122 jobs that have run with the updated json output (write the computed monitor delay), I found 17 that were assigned the default value (at the time 0.351). When I investigated each of these, I discovered that there were 1 more "stimulus transitions" (vsyncs sampled at the frame rate) than "photodiode events". When I truncated the "transitions" array to the size of the photodiode events, the monitor delay was computed as expected (~0.021). In the same time_sync module, ophys timestamps are truncated if the length of the ophys timestamps is greater than the length of the data in the |
kschelonka
added a commit
that referenced
this issue
Mar 6, 2020
If there are more "stimulus transitions" than "photodiode events", truncate the "stimulus transitions" to the number of "photodiode events". This is similar to what is done with ophys timestamps data in this module. Update test coverage to handle this case. Update logging to show the min and max delay, to help with debugging in the case of unusual/default values.
kschelonka
added a commit
that referenced
this issue
Mar 6, 2020
If there are more "stimulus transitions" than "photodiode events", truncate the "stimulus transitions" to the number of "photodiode events". This is similar to what is done with ophys timestamps data in this module. Update test coverage to handle this case. Update logging to show the min and max delay, to help with debugging in the case of unusual/default values.
|
If we decide to merge this PR, I think we should also update the job to no longer provide a default value (and instead raise an error if there is an issue). |
kschelonka
added a commit
that referenced
this issue
Mar 10, 2020
If there are more "stimulus transitions" than "photodiode events", truncate the "stimulus transitions" to the number of "photodiode events". This is similar to what is done with ophys timestamps data in this module. Update test coverage to handle this case. Update logging to show the min and max delay, to help with debugging in the case of unusual/default values.
|
Spoke with Doug, he is recommending an overhaul of our photodiode event extraction algorithm. His recommendation works for most cases but does fail in certain instances, so I think there will be a lot of analysis and validation work. I think we should update this story to the next milestone and increase the estimate. |
kschelonka
added a commit
that referenced
this issue
Mar 17, 2020
Previously we used consective photodiode edges of the same type
to determine whether an event is a valid transition. This led
sometimes to missing out on the final valid event, and resulted
in an error ("stimulus transitions" and "photodiode events" did
not have the same length).
The algorithm has been updated to use consecutive events as
described below:
From @dougo:
1. Every transition from rise/fall or fall/rise represents a
change in the state of the sync square. Call these ‘photodiode events’
2. During the experiment, the sync square changes state every N
frames (where N = 60 by default). Let’s call these ‘sentinel frames’
3. A transition marks a ‘sentinel frame’ if the next photodiode
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’
He also notes that the time delay between each ‘valid transition’
and each ‘sentinel frame’ should be roughly constant (mean
between 20 and 40 ms, std of just a few ms for a given experiment)
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
added a commit
that referenced
this issue
Mar 17, 2020
Previously we used consective photodiode edges of the same type
to determine whether an event is a valid transition. This led
sometimes to missing out on the final valid event, and resulted
in an error ("stimulus transitions" and "photodiode events" did
not have the same length).
The algorithm has been updated to use consecutive events as
described below:
From @dougollerenshaw:
1. Every transition from rise/fall or fall/rise represents a
change in the state of the sync square. Call these ‘photodiode events’
2. During the experiment, the sync square changes state every N
frames (where N = 60 by default). Let’s call these ‘sentinel frames’
3. A transition marks a ‘sentinel frame’ if the next photodiode
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’
He also notes that the time delay between each ‘valid transition’
and each ‘sentinel frame’ should be roughly constant (mean
between 20 and 40 ms, std of just a few ms for a given experiment)
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
added a commit
that referenced
this issue
Mar 20, 2020
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
added a commit
that referenced
this issue
Mar 20, 2020
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
added a commit
that referenced
this issue
Mar 20, 2020
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
added a commit
that referenced
this issue
Mar 27, 2020
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
added a commit
that referenced
this issue
Mar 27, 2020
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
added a commit
that referenced
this issue
Mar 27, 2020
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
added a commit
that referenced
this issue
Mar 27, 2020
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
added a commit
that referenced
this issue
Mar 27, 2020
GH-1404: Handle monitor delay errors
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
One example:
/allen/programs/braintv/production/visualbehavior/prod4/specimen_975406504/ophys_session_1008813715/ophys_experiment_1009114207/202002210341_OPHYS_TIME_SYNC_QUEUE_1009114207.log
To determine whether we should throw an error and fail the job. Currently a warning is logged. The monitor delay gets added to lims somewhere (?)
The text was updated successfully, but these errors were encountered: