Skip to content

Fix plotly code - #106

Merged
rachelstephlee merged 14 commits into
mainfrom
fix_plotly_code
Jul 1, 2026
Merged

Fix plotly code#106
rachelstephlee merged 14 commits into
mainfrom
fix_plotly_code

Conversation

@rachelstephlee

@rachelstephlee rachelstephlee commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

improving some of the plotly code:

for plot_foraging_session_plotly
-> Adding a plot_foraging_session_nwb_plotly version that takes in an NWB instead of choice/reward history.

for plot_session_in_time_plotly:
->Added a plot_session_in_time_nwb_plotly that takes in an NWB instead of tidy dataframes. This will match plot_session_scroller better.
-> allows user to pick which FIP to use (G_1_preprocessed is outdated channel name)


def plot_session_in_time_plotly( # noqa: C901 pragma: no cover
df_events, df_trials=None, fip_df=None, adjust_time=True, title=None, smooth_factor=5
nwb_list, fip=[], adjust_time=True, title=None, smooth_factor=5

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.

Instead of changing the API from tidy dfs to nwb_list, could you create a small wrapper that prepares the dfs from nwb_list (essentially your code from L581-614)? Two considerations:

  1. separating data preparation and plotting is always a good practice
  2. to keep backward compatibility for the use case here, where one can call this function from other data sources than nwb_util

@rachelstephlee rachelstephlee Jun 30, 2026

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.

the main issue is the lack of fip list. users should be able to plot the channels that they want. plus, G_1_preprocessed is an outdated preprocessing suffix for the channels...!

Also, if we're matching "plot_session_in_time," it's taking in an nwb and a list of FIP, not dataframes.

@alexpiet , thoughts?

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 confused, it seems like we are talking about two issues - whether to have a wrapper function that accepts a list of NWB files, and how to input which FIP channels to plot?

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.

I decided that there should always be an input for FIP channels to plot (the default would have plotted no FIP channels unless we rename channels).

I did add the wrapper function after all to help preserve han's use case.

@rachelstephlee

Copy link
Copy Markdown
Collaborator Author

Han, I've updated with the wrapper function so you can keep your functionality.

I did change the FIP plotting because it was wrong with the default fip_channel list and it should allow users to pick the channels.

Since this is just to mirror the function (and not exactly match) I think having a wrapper is fair. I'll wait until Alex gets back with his thoughts.

@rachelstephlee
rachelstephlee requested a review from hanhou June 30, 2026 03:03

@hanhou hanhou left a comment

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 tested on my side and it works. Thanks!

@rachelstephlee
rachelstephlee requested a review from alexpiet July 1, 2026 18:17
@rachelstephlee
rachelstephlee merged commit 8064a12 into main Jul 1, 2026
2 checks passed
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.

3 participants