-
Notifications
You must be signed in to change notification settings - Fork 0
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 condition_labels
as an argument
#18
Conversation
if condition_labels is None: | ||
condition_labels = np.array([f"condition_{i}" for i in range(number_of_conditions)], dtype="U") |
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.
why generate if they don't exist? Why not just make them optional?
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'd rather not add data if it is redundant and does not include any new information
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.
Assuming people want a mock with data if they specify num_conditions > 0
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 I'd really rather not. It's auto-magic, which is hard for users to predict, and it is duplication of data.
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.
Sure. Don't feel very strong about this. Removing.
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.
But your comment of duplication of data I don't understand. What is being duplicated?
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.
"condition 0" is at index 0
"condition 1" is at index 1
etc.
I guess it's not really duplicating since that data does not already exist, but it's adding data that is not really telling us anything we couldn't already infer. I think it's more meaningful to be able to tell whether conditions are labeled or not.
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.
Makes sense, I was just wondering if I missed something. I eliminated this behavior for the mock in the last commit.
…' into add_condition_label
* test for condition-less case working * tests for multiple conditions * documentation * ruff * typos * Update spec/ndx-binned-spikes.extensions.yaml * timestamps to event timestamps * ruff and spelling * Add `condition_labels` as an argument (#18) * add condition labels * Update src/pynwb/ndx_binned_spikes/__init__.py * Update spec/ndx-binned-spikes.extensions.yaml * remove automatic creation of labels in the mock * typo on the spec generation --------- Co-authored-by: Ben Dichter <[email protected]> --------- Co-authored-by: Ben Dichter <[email protected]>
A request from @bendichter in #17.
Separated and with #17 as its base to simplify the discussion.