Skip to content

Psth multiple nwbs#70

Closed
rachelstephlee wants to merge 10 commits into
mainfrom
psth-multiple-nwbs
Closed

Psth multiple nwbs#70
rachelstephlee wants to merge 10 commits into
mainfrom
psth-multiple-nwbs

Conversation

@rachelstephlee

Copy link
Copy Markdown
Collaborator

PSTH for mulitple NWBs

@rachelstephlee rachelstephlee requested a review from alexpiet August 9, 2025 00:13
"""
if not hasattr(nwb, "df_fip"):
# Check if nwb is a list, and if so, check only the first element for attributes and channel
nwb_to_check = nwb[0] if isinstance(nwb, list) else nwb

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why check only the first NWB? Checks are fast, so we should just check all of them

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

agreed. done. am testing and wll send PR back to you once I do that.

censor_times.append(align_dict[key])
if isinstance(nwb, list):
align_dict_flat[key] = np.concatenate(align_dict[key])
censor_times.append(align_dict_flat[key])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what you are doing here with align_dict_flat

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This feels wrong, because the censor times from one NWB shouldn't impact the censor times of another NWB

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

you're right-- fixed.

# Check if nwb is a list, and if so, check only the first element for attributes and channel
nwb_to_check = nwb[0] if isinstance(nwb, list) else nwb

if not hasattr(nwb_to_check, "df_fip"):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Again, I would check all NWBs

@rachelstephlee

Copy link
Copy Markdown
Collaborator Author

Thank you for the feedback.

i have:

  1. checked all nwbs in the list of nwbs for the correct df_fip, df_events, df_trials as needed
  2. checked that there are multiple alignments, censor_times if multiple nwbs are passed in
  3. fixed the censor_times calculation so it is per nwb if multiple nwbs are passed in.

i have tested the functions with:

  1. single nwbs with alignments as column names and dictionaries/lists of timepoints to ensure that has not been broken
  2. same as 1, but with multiple nwbs passed in.

return
else:
align_vals = nwb_i.df_events.query("event == @a")["timestamps"].values
align_list = align_dict.get(a, [])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why save things to align_vals, then use .get()? You already checked above if a is in df_events

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

to not repeat it for the if/else logic below.

.get is checking align_dict, not checking nwb_i.df_events.

align_vals = nwb_i.df_events.query("event == @a")["timestamps"].values
align_list = align_dict.get(a, [])
if len(nwb_to_check) > 1:
align_list.append(align_vals)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Whats going on here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

there are two cases:

one single nwb coming into plot_fip_psth_compare_alignments, in which case the align_dict should have a string key and a list of the align_vals.

multiple nwb's coming into 'plot_fip_psth_compare_alignments', in which case the align_dict should have a string key and a list of lists (each list an align_val for each NWB)

If I don't separate out the two cases when looking at alignments given as a column, the align_dict for a single NWB will have a string key and a nested list (list of a single list). This will cause a problem in line 95, because np.concatenate will fail.

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