Skip to content

Conversation

@hargunkaur286
Copy link

@hargunkaur286 hargunkaur286 commented Apr 2, 2025

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

This PR adds functionality to automatically generate and save probe plots using the ProbeInterface library. The probe plots are saved at the run level (in a dedicated probe_plots folder) when save_preprocessed() is run. In addition, a session-level function (plot_probe()) is exposed to quickly visualize the probe associated with the entire session.

What does this PR do?

  1. Introduces a new save_probe_plot() method in the PreprocessedRun class which:
  • Retrieves the probe information from the final preprocessed recording (using get_probe()).
  • Creates a Matplotlib figure and axis explicitly.
  • Calls probeinterface.plotting.plot_probe with the provided axis.
  • Saves the generated probe plot image to a subfolder (probe_plots) within the run folder.
  1. Updates the run-level save_preprocessed() method to automatically call save_probe_plot() after saving the preprocessed recordings.
  2. Adds a session-level plot_probe() function in the Session class.

References

Fixes #197

How has this PR been tested?

  • Ran the full preprocessing pipeline on the example dataset using python3 run_example.py.
  • Verified that for each run (e.g., run-001_g0_imec0 and run-002_g0_imec0), a probe plot image (probe_plot.png) is created in the corresponding probe_plots folder.
  • Confirmed that no existing functionality (preprocessing, saving, plotting of preprocessed traces) was broken by these changes.

Is this a breaking change?

If this PR breaks any existing functionality, please explain how and why.

Does this PR require an update to the documentation?

If any features have changed, or have been added. Please explain how the
documentation has been updated.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@adamltyson
Copy link
Member

Thanks for this @hargunkaur286, just FYI due to the main developer of spikewrap being away, it may be some time until this can be reviewed.

@adamltyson adamltyson requested a review from JoeZiminski April 2, 2025 15:05
@hargunkaur286
Copy link
Author

Thanks for this @hargunkaur286, just FYI due to the main developer of spikewrap being away, it may be some time until this can be reviewed.

Hi @adamltyson,
Thank you for the update. I saw the note on the previous PR (#231) mentioning that @JoeZiminski is away, so I completely understand. I’ll wait patiently for the review. Thanks again!

Copy link
Member

@JoeZiminski JoeZiminski left a comment

Choose a reason for hiding this comment

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

Hey @hargunkaur286 thanks a lot for this! Very nice approach, please see suggestions mostly around refactoring.

Signed-off-by: Hargun Kaur <[email protected]>
@hargunkaur286
Copy link
Author

Hello @JoeZiminski ,
I've implemented the requested updates related to probe plotting and integration.

I added a get_probe() method to the PreprocessedRun class, which returns a dictionary of probe objects indexed by shank ID (e.g. "grouped", "shank_0", etc.) like you suggested. Also, I moved the probe plotting logic to a new method Session.plot_probe() to ensure that all runs have consistent probe structures before plotting and it handles both grouped and per-shank cases.

Signed-off-by: Hargun Kaur <[email protected]>
Signed-off-by: Hargun Kaur <[email protected]>
@JoeZiminski
Copy link
Member

Hey @hargunkaur286 thanks a lot for this, this is looking good thanks also for the tests. Currently I'm on paternity leave so will not have time to review and test this in full prior to the GSoC deadline, of course do include this contribution in your application nonetheless.

Some quick feedback, I would suggest removing all plotting functionality from the _preprocessing_run class and only expose a simple getter. Plotting / saving can be handled entirely at the session level (we are unlikely to ever save a separate probe plot for each run, as they should be the same for all runs within a session. Cheers!

@hargunkaur286
Copy link
Author

Hey @hargunkaur286 thanks a lot for this, this is looking good thanks also for the tests. Currently I'm on paternity leave so will not have time to review and test this in full prior to the GSoC deadline, of course do include this contribution in your application nonetheless.

Some quick feedback, I would suggest removing all plotting functionality from the _preprocessing_run class and only expose a simple getter. Plotting / saving can be handled entirely at the session level (we are unlikely to ever save a separate probe plot for each run, as they should be the same for all runs within a session. Cheers!

Thanks a lot for the quick feedback, really appreciate it! I've updated the implementation to remove plotting logic from _preprocess_run and now handle all probe plotting/saving at the session level as suggested. Thank you again, and enjoy your paternity leave! Meanwhile, I'll try to solve other open issues :)

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.

Automatically save plot_probe

3 participants