-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add check for series regularity when passing timestamps in VideoInterface
#676
base: main
Are you sure you want to change the base?
Conversation
VideoInterface
The intention is that if you are bothering to explicitly set Real question is, if you're fetching and setting a vector of timestamps in this case, why do you want to check if they are regular after the fact? |
People might get the correct timestamps from somewhere else and they set them but they don't know they are regular. Outside of that, we do the checks everywhere to never write regular timestamps. No cost to do it here as well. I see no gain from making assumptions about user intentions or knowledge: if the timestamps are regular, we write according to best practices. |
Specific to video data, this does not hold true Both DLC and SLEAP appear to use timestamps from their original videos: And see this PR from an external user exposing that control on our side: #532
My point is that if a user did that, they might be confused why their output NWB file did not contain any timestamps (and was magically transformed to a Hence why I've been pushing for the following structure... -> If user specifies nothing, let format automatically determine. Could be irregular timestamps, a constant rate, or a constant rate inferred from regular timestamps (but point is user is unaware of any of it) -> If user specifies timestamps via the alignment methods, then use the timestamps they give (maybe we could check for regularity and warn?) -> If user wants to override
Minor RAM/CPU cost for very long videos. If user knows what they want to do ahead of time, no sense wasting the operation CORRECTION: |
Is your position that the user intention should be held at a higher level than the best practices to minimize surprise when they see the files? Do you want to defend that in general or just for this specific point of timestamps? We already make some decisions on their behalf like compressing data. We should discuss this on the general meeting if you want to push this agenda. My initial reaction is rejection, I think it is very hard to make good assumptions about user intention and knowledge but consistent behavior (i.e. we always write data according to best practices) is easier to document and convey. Maybe there are some benefits or costs that I am not seeing. You should be aware that the we are currently taking the opposite policy with the recording extractors from spikeinterface though. The only reason that a RecordingExtractor would have timestamps is because the user set those timestamps themselves. And in that case even if they did, we do not write them if they are regular. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #676 +/- ##
==========================================
- Coverage 92.06% 92.05% -0.02%
==========================================
Files 115 115
Lines 5963 5968 +5
==========================================
+ Hits 5490 5494 +4
- Misses 473 474 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I am using the method
set_aligned_timestamps
in the VideoInterface but then there is no check for series regularity for this case:neuroconv/src/neuroconv/datainterfaces/behavior/video/videodatainterface.py
Lines 340 to 341 in 5687c07
I just added that and removing one inlined definition that was getting in the way of my debugging.