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

Simplify structure #17

Merged
merged 11 commits into from
Sep 3, 2024
Merged

Simplify structure #17

merged 11 commits into from
Sep 3, 2024

Conversation

h-mayorquin
Copy link
Collaborator

Follow up from #16

Uses a single structure instead of two as in the aferomentioned PR.

A good place to start the review will be to read the documentation:
https://github.com/catalystneuro/ndx-binned-spikes/tree/simplify_structure

@h-mayorquin h-mayorquin self-assigned this Aug 21, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review August 21, 2024 18:11
@bendichter
Copy link
Contributor

This is looking good. One question I have is: what are the condition_indices indexing? In some cases, maybe 0,1,2,3 makes sense, but I think often it might make more sense to list the conditions. Maybe we could add a conditions array that is a list of strings. For example, in the IBL dataset the condition may indicate the different conditions for stimulus constrast. For the DiCarlo datasets, it may indicate the filenames of the unique visual stimuli.

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Aug 21, 2024

One question I have is: what are the condition_indices indexing? In some cases, maybe 0,1,2,3 makes sense, but I think often it might make more sense to list the conditions. Maybe we could add a conditions array that is a list of strings. For example, in the IBL dataset the condition may indicate the different conditions for stimulus constrast. For the DiCarlo datasets, it may indicate the filenames of the unique visual stimuli.

When I was re-writing this I was thinking that we could add an extra column for the conditions. I was thinking that we could make that extra column a link to either indexed stimuli or a column of a dynamic table or whatever.

But now, I think what you proposal is simpler and maybe better. Just pass a vector with conditions as string as you mention. Note that with this could also make condition_indices an internal implementation detail.

README.md Outdated Show resolved Hide resolved
README.md Outdated
milliseconds_from_event_to_first_bin = -50.0 # The first bin is 50 ms before the event
bin_width_in_milliseconds = 100.0
name = "BinnedAignedSpikesForMyPurpose"
description = "Spike counts that is binned and aligned to events."
binned_aligned_spikes = BinnedAlignedSpikes(
data=data,
event_timestamps=event_timestamps,
timestamps=timestamps,
bin_width_in_milliseconds=bin_width_in_milliseconds,
milliseconds_from_event_to_first_bin=milliseconds_from_event_to_first_bin,
description=description,
Copy link
Contributor

Choose a reason for hiding this comment

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

this does not have event conditions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I introduce the possibility of storing multiple conditions in the next section. I think it makes presentation easier if less concepts are introduced at the same time.

@bendichter
Copy link
Contributor

bendichter commented Aug 29, 2024

I really think it could use an optional argument condition_labels, which are string labels for each of the condition_indices. I think this is particularly true if you are going to use the term "indices". If you use that term, then those integers must be indexing something. If you add a condition_labels, then this is the dataset that is being indexed.

@h-mayorquin
Copy link
Collaborator Author

I really think it could use an optional argument condition_labels, which are string labels for each of the condition_indices. I think this is particularly true if you are going to use the term "indices". If you use that term, then those integers must be indexing something. If you add a condition_labels, then this is the dataset that is being indexed.

Let's do this!

@h-mayorquin
Copy link
Collaborator Author

@bendichter this should be ready. I separated the condition_labels request in #18 to simplify the discussion. Let me know what you think.

* 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]>
@h-mayorquin h-mayorquin merged commit 5470bde into main Sep 3, 2024
30 checks passed
@h-mayorquin h-mayorquin deleted the simplify_structure branch September 3, 2024 22:23
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