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

Added no harp line warning as red title on the plot #3

Merged
merged 9 commits into from
Aug 1, 2024

Conversation

ZhixiaoSu
Copy link
Collaborator

@ZhixiaoSu ZhixiaoSu commented Jul 31, 2024

This PR made two changes:

  1. When there's no harp clock detected, there will be error log in the printed output and also a red title on the harp line plot like this: harp_line_search
  2. Save parameters in output file to enable potential debugging.
    2024-07-31 11:30:17 Running generate_report.py with parameters: report_name: QC.pdf timestamp_alignment_method: harp original_timestamp_filename: original_timestamps.npy plot_drift_map: False flip_NIDAQ: False

@ZhixiaoSu ZhixiaoSu marked this pull request as ready for review July 31, 2024 18:18
@ZhixiaoSu ZhixiaoSu marked this pull request as draft July 31, 2024 18:54
@ZhixiaoSu ZhixiaoSu marked this pull request as ready for review July 31, 2024 19:08
@ZhixiaoSu
Copy link
Collaborator Author

Hi @jsiegle and @alejoe91, this is ready for review. It's a few small fixes based on some recent feedbacks. One thing I'm not sure is: I had to hold the spikeinterface package to an older version to make sure the package works. For example, se.get_neo_streams("openephys", directory) won't work in lastest version in spikeinterface as 'openephys' is not one of the extensions. Maybe we can update the repo to be compatible to lastest version of spikeinterface later?

@alejoe91
Copy link
Collaborator

@ZhixiaoSu in the new version this has slightly changed: openephys should be openephysbinary (which is more precise ;))

Copy link
Collaborator

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ZhixiaoSu
Copy link
Collaborator Author

LGTM!

Thank you! I'll upgade spikeinterface next time.

@ZhixiaoSu ZhixiaoSu merged commit cde7831 into main Aug 1, 2024
4 checks passed
@ZhixiaoSu ZhixiaoSu deleted the no_harp_warning branch August 1, 2024 17:21
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.

2 participants