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.
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
Support digital channels on NIDQ interface and use TimeSeries instead of ElecricalSeries for analog channels #1152
Support digital channels on NIDQ interface and use TimeSeries instead of ElecricalSeries for analog channels #1152
Changes from 21 commits
58120d6
b560f29
ca00c34
1b350f1
683ba39
25e6486
78b9132
f12a900
79ac49d
e702a58
71b3299
a965d23
e49618e
f54a08e
d452910
c4afd31
7cacf76
df512de
e0fd5ca
dae7ae3
7287a5d
7f78db9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the behavior change? What should be done instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved the warning to give more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should es_key still be here? "es" stands for ElectricalSeries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I opened an issue here:
#1154
I would like to hear your thoughts on it when you have some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I updated all the warning strings to add context and information. Take a look and let me know what you think.