added get_average_signal_window.#69
Conversation
|
Checked for bugs, added check for nwb.df_trials. The bug was subtle but important. when Censor = True, the alignment ocde sorts the event_times. If Nan values are in event_times (for example, if aligned to choice_time, choice_time = Nan for trials in which the mice made no choice), the Nan values are automatically sorted to the end. Since the code assumes alignment_times should be in df_trials, so therefore should be ordered in trials not by time. If Nan values are shuffled to the end, the ordering of averaged times --> trials will be messed up. I didn't want to change alignment.py because this is the correct behavior for any event_times. Let me know if you feel differently-- we can check for nan values in event_times and then order them according to their original position (but would that even make sense?). |
|
I think the better solution is to remove NaNs in the first place:
I would also call |
both done (dropping na helped with that)
done. added censoring as an option.
Done. this was really helpful and made it much cleaner. |
|
Addressed comments and checked functionality. I added a wrapper function for multiple NWBs as well.. I was working on it concurrently, so I figured we can put it in as well. |
|
|
||
| if not hasattr(nwb, "df_trials"): | ||
| raise ValueError("You need to compute df_trials: nwb_utils.create_trials_df(nwb)") | ||
|
|
There was a problem hiding this comment.
You should add a check here that df_fip has been computed, and reference the code to do it:
if not hasattr(nwb, "df_fip"):
raise AttributeError("You need to compute df_fip: nwb_utils.create_fib_df(nwb)")
| df_trials = nwb.df_trials.dropna(subset=alignment_event, inplace=False) | ||
| df_trials = df_trials.sort_values(by=alignment_event) | ||
|
|
||
| data = nwb.df_fip.query("event == @channel") |
There was a problem hiding this comment.
You should check here that data is not empty. This could happen if the channel isn't in the df
There was a problem hiding this comment.
done. added a check that the channel is in the df_fip.
There was a problem hiding this comment.
should i just return an empty df_trials if the channel isn't present or give a value error if channel isn't in the df_fip?
just thinking of cases when i am running this for multiple NWB's, i might have some channels that exist for a particular NWB vs not. In that case, i might want to skip over that call rather than return an error.
There was a problem hiding this comment.
I think I would either:
- generate an error
- return the unchanged original df_trials, and then generate a warning about the missing channel
There was a problem hiding this comment.
or, I guess I see that in the multiple session version you are concatenating the df_trials, so an empty dataframe is fine
| # Check alignment_event is in df_trials columns | ||
| if alignment_event not in nwb.df_trials.columns: | ||
| raise ValueError(f"alignment_event '{alignment_event}' not found in df_trials columns.") | ||
|
|
There was a problem hiding this comment.
Add a check that data_column is in the dataframe
| output_col = ( | ||
| f"{data_column}_{channel}_{offsets[0]}_" | ||
| f"{offsets[1]}_{alignment_event.replace('_in_session','')}" | ||
| ) |
There was a problem hiding this comment.
Should you add a check that the output column isn't already in the dataframe?
There was a problem hiding this comment.
if user provides an output_col "Average_signal" but uses different data_columns, channels, and offsets, we should allow them to update it.
I would rather keep this as is if it's OK
alexpiet
left a comment
alexpiet
left a comment
There was a problem hiding this comment.
Add additional checks for input
alexpiet
left a comment
alexpiet
left a comment
There was a problem hiding this comment.
Looks good, thanks for the fixes
2 participants
get_average_signal_window takes in an nwb and takes the average window of signal around an alignment event that occurs once per trial.