Compute display lag in BehaviorOphysLimsApi #1400
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously the BehaviorOphysLimsApi did not compute monitor delay, but added a default value of 0.351. This updates the default to 0.215, which is the average of monitor delay for a large number of recent datasets. Additionally use the OphysTimeAligner methods to compute the display lag, which is used in the OphysTimeSync job to produce corrected timestamps in the LIMS pipeline.
|
Check out this pull request on You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB. |
Codecov Report
@@ Coverage Diff @@
## rc/1.6.0 #1400 +/- ##
============================================
+ Coverage 34.59% 34.71% +0.12%
============================================
Files 342 342
Lines 33248 33250 +2
============================================
+ Hits 11501 11542 +41
+ Misses 21747 21708 -39
Continue to review full report at Codecov.
|
kschelonka
requested review from
NileGraddis,
nickponvert and
dougollerenshaw
nickponvert
approved these changes
Feb 29, 2020
| @@ -15,7 +15,7 @@ | |||
| REG_PHOTODIODE_MAX = 2.1 # seconds | |||
| PHOTODIODE_ANOMALY_THRESHOLD = 0.5 # seconds | |||
| LONG_STIM_THRESHOLD = 0.2 # seconds | |||
| ASSUMED_DELAY = 0.0351 # seconds | |||
| ASSUMED_DELAY = 0.0215 # seconds | |||
There was a problem hiding this comment.
I'm cautiously confident that this value is better than 0.0351 based on some analysis Wayne and I did. However, there should be a mechanism in place to re-evaluate this every so often as we collect more data to make sure this is still a reasonable value.
| @@ -38,8 +39,10 @@ def get_sync_data(self): | |||
|
|
|||
| @memoize | |||
| def get_stimulus_timestamps(self): | |||
| monitor_delay = .0351 | |||
| return self.get_sync_data()['stimulus_times_no_delay'] + monitor_delay | |||
| sync_path = self.get_sync_file() | |||
kschelonka
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Response to #900
I updated the default value for monitor delay from .0351 to .0215, per the output of the ophys time sync jobs.
I also used the OphysTimeAligner class to compute the monitor delay, which is used by the ophys time sync job. This will throw a warning if the monitor delay falls outside of our "expected" parameters, but will still use the default value.
Looking for some feedback. Should we throw an error instead of a warning? What about the ophys time sync job?Edit: we should investigate this, but can be done outside of this story.I did some investigation and found that the stimulus timestamps were not being treated in a rig specific way (I was thinking of the ophys timestamps). So the 2p2 rigs either have a longer monitor delay for whatever reason than our other rigs, or there is some other issue causing the delay to be computed incorrectly.