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

Add ignore_timestamps_errors to OpenEphys interfaces #579

Closed
wants to merge 8 commits into from

Conversation

weiglszonja
Copy link
Contributor

@weiglszonja weiglszonja commented Sep 26, 2023

To fix catalystneuro/tye-lab-to-nwb#29

@weiglszonja weiglszonja marked this pull request as draft September 26, 2023 12:14
@weiglszonja weiglszonja self-assigned this Sep 26, 2023
@weiglszonja
Copy link
Contributor Author

when ignore_timestamps_errors is used then we should flag somehow (warn the user that we are unable to infer timestamps automatically) but don't set times automatically for those when this error would have been raised.

Comment on lines 77 to 84
if ignore_timestamps_errors and self.recording_extractor.has_time_vector():
for segment_index in range(self.recording_extractor.get_num_segments()):
self.recording_extractor._recording_segments[segment_index].time_vector = None
warn(
"The recording has a time vector but the timestamps might be discontinuous. "
"The time vector was reset to None to avoid writing these timestamps with the electrical series."
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodyCBakerPhD how about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitively seems perfectly fine - only thing to check is if the logic on the SI side relies on value of the .time_vector is None rather than on the logic hasattr(recording_segment, "time_vector")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No matter what we have time to polish this; this shouldn't be merged to main until both neo and SI get new releases with the upstream fixes

Tye lab repo will have to pin to either this branch (to stay up-to-date with other things updated from main) or a commit from this branch

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #579 (49ee77b) into main (214ff88) will decrease coverage by 0.05%.
Report is 1 commits behind head on main.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #579      +/-   ##
==========================================
- Coverage   90.78%   90.74%   -0.05%     
==========================================
  Files         103      103              
  Lines        5374     5378       +4     
==========================================
+ Hits         4879     4880       +1     
- Misses        495      498       +3     
Flag Coverage Δ
unittests 90.74% <62.50%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rfaces/ecephys/openephys/openephysdatainterface.py 100.00% <ø> (ø)
.../ecephys/openephys/openephyslegacydatainterface.py 90.90% <62.50%> (-6.60%) ⬇️

@CodyCBakerPhD
Copy link
Member

@weiglszonja Can you remind me; do we still need or even want this exposure, or is the plan still to try to fix neo or the underlying violating filess more fundamentally?

@weiglszonja
Copy link
Contributor Author

@CodyCBakerPhD I think we shouldn't expose it for now, can be closed. The plan is to fix the underlying files

@weiglszonja weiglszonja closed this Dec 7, 2023
@CodyCBakerPhD CodyCBakerPhD deleted the add_ignore_timestamps_errors_to_interface branch December 7, 2023 18:33
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

Successfully merging this pull request may close these issues.

Discontinuous timestamps in AUX.continuous data files
3 participants