Support digital channels on NIDQ interface and use TimeSeries instead of ElecricalSeries for analog channels #1152
Conversation
This reverts commit 1b350f1.
h-mayorquin
changed the title
| warnings.warn( | ||
| "The load_sync_channel parameter is deprecated and will be removed in June 2025.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
What is the behavior change? What should be done instead?
There was a problem hiding this comment.
The last channel of the ap and lf bands of SpikeGLX binaries are used to store synch data. This command does not make sense with the NIDQ interface because there is no such channel. To be honest, I don't know what role was playing here before.
There was a problem hiding this comment.
Improved the warning to give more context.
| from spikeinterface.extractors import SpikeGLXEventExtractor | ||
|
|
||
| self.event_extractor = SpikeGLXEventExtractor(folder_path=self.folder_path) | ||
|
|
||
| super().__init__( | ||
| verbose=verbose, | ||
| load_sync_channel=load_sync_channel, | ||
| es_key=es_key, |
There was a problem hiding this comment.
should es_key still be here? "es" stands for ElectricalSeries
There was a problem hiding this comment.
Yes, I think you are right.
At the moment, it is not clear how to pass metadata to this object but also there is not clear metadata to be passed. I discussed yesterday with a user that has this type of data about what could be useful for their use case and the only thing that came up is a description of the channels.
I will open another issue to discuss this.
Related, I really like the proposal that you had here:
#629
There was a problem hiding this comment.
OK, I opened an issue here:
#1154
I would like to hear your thoughts on it when you have some time.
| if starting_time is not None: | ||
| warnings.warn( | ||
| "The 'starting_time' parameter is deprecated and will be removed in June 2025.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
|
|
||
| if write_as != "raw": | ||
| warnings.warn( | ||
| "The 'write_as' parameter is deprecated and will be removed in June 2025.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) | ||
|
|
||
| if write_electrical_series is not True: | ||
| warnings.warn( | ||
| "The 'write_electrical_series' parameter is deprecated and will be removed in June 2025.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
For each of these, it would be helpful to provide a bit more explanation about why it is being deprecated, and/or what the new behavior is, and/or what to do to get the same result instead of using this arg
There was a problem hiding this comment.
I think you are right that's a good practice.
To add more context, those are conversion options that only made sense for a general electrical series and did not even make sense for the NIDQ interface. For example, the write_as is a general argument to decide whether we add this in acquisition or as a processing module but in this case we are sure NIDQ is acquisition so the choice does not make sense.
In other words, if someone played with those parameters for the nidq interface they were writing wrong data. I don't think that people did but just to be safe I ported the signature as it was so their code will not break immediately.
All of that said, I think that this type of information should be included in the warning. It is useful and it would have avoided this discussion at the review, my bad.
There was a problem hiding this comment.
yeah, that makes sense. I just think we need a sentence or two alluding to this for users who are less familiar and may be copying/pasting old code
There was a problem hiding this comment.
Yes, I updated all the warning strings to add context and information. Take a look and let me know what you think.
|
This looks great! Thanks for putting this together. Just a minor request about the warn messages |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1152 +/- ##
==========================================
+ Coverage 90.69% 90.76% +0.06%
==========================================
Files 129 129
Lines 8189 8281 +92
==========================================
+ Hits 7427 7516 +89
- Misses 762 765 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This should come after #1150 and has it as a base.
This should close #994 and also should close #923