Skip to content

Conversation

bparks13
Copy link
Member

Overview

This PR refactors how probe configuration data is stored and managed across Neuropixels 1.0, Neuropixels 2.0, and RHS2116 devices. The main changes involve externalizing probe interface files into JSON format and improving error handling around file operations.

Key Changes

  • Moved calibration file paths into respective ProbeConfiguration classes for better encapsulation
  • Added ProbeInterfaceFile properties to manage external JSON configuration files
  • Improved error handling for file operations with specific exception types
  • Renamed UI elements from "Channel Configuration" to "Probe Group" for consistency
  • Added GenericPropertyConverter for better property management in UI
  • Cleaned up unused dependencies and imports

Breaking Changes

  1. Calibration file properties have been moved:

    • From ConfigureNeuropixelsV1/ConfigureNeuropixelsV2 classes to their respective ProbeConfiguration classes
    • Old properties are marked as obsolete and will be removed in v1.0.0
  2. Configuration files are now stored externally:

    • JSON files with pi_*.json extension for automatically created file paths
    • Generated automatically when saving Bonsai workflows
    • Can be manually specified via ProbeInterfaceFile property

Migration Guide

For existing workflows:

  1. Calibration files will be automatically migrated to the new ProbeConfiguration location
    • This is only true for Neuropixels 1.0f and Neuropixels 2.0e/Beta probes. Due to how the properties are organized for Neuropixels 1.0e they cannot be automatically migrated.
  2. Probe interface files will be generated automatically next to .bonsai files
  3. No manual changes required for existing configurations

Fixes #366

@bparks13 bparks13 added this to the 0.7.0 milestone Aug 11, 2025
@bparks13 bparks13 requested a review from jonnew August 11, 2025 20:15
- Removed the #nullable enable preprocessor directives
- Changed public property to be ProbeInterfaceFileName
- Remove private backing field, and also remove custom logic to enforce .json extension in manually typed filenames
- Added SaveFileNameEditor to all ProbeInterfaceFileName properties
- Modified comments to be more explicit in that the obsolete properties/methods will be removed in 1.0.0
@jonnew
Copy link
Member

jonnew commented Aug 22, 2025

Although we discussed this, this filename is pretty unappealing to me:

image

What about

pi_<probe-sn>.json

Because this thing has nothing to do with ONI, really, right? or maybe

pi_<probe type>_<probe-sn>.json

both of these are available on the eeprom on the probe

Edit: I guess the the reason is that we want the user to be able to save these files before adding a calibration file and certainly before connecting to hardware a required for EEPROM. Given that, i would do the following:

pi_oni-256_<configuration class name>.json

and skip the headstage which I dont think is helpful

@jonnew
Copy link
Member

jonnew commented Aug 22, 2025

Can this display the default file name, greyed out, in the text box if its null?

image

If too annoying (like requires logic like the PortVoltage stuff does) then dont worry about it

@jonnew
Copy link
Member

jonnew commented Aug 22, 2025

Something fields incoherent about this:

image
  1. The ProbeInterfaceFile is external file you can specify to determine an electrode layout. Its a top level member of the ConfigureNeuropixels1e
  2. The calibration files are external files you can you use to determine parameters for your probe. They are 2nd order properties inside ProbeConfiguration
  3. When you open the GUI there are two completely different ways to handle these for historical reasons and decisions. Additionally different terminology is used: ProbeInterfaceFile versus Probe Group.

I think we need to examine a coherent approach here.

With respect to 1 and 2, why is one file a top level property and the other not? It feels like they should all be inside probe configuration?

With respect to 3, the naming conventions need to be unified and the way things are loaded. Before we had all this import stuff because we had our own internal representation of probe interface inside bonsai file. Now we are just straight up using probe interface files so there is no importing etc as far as I can see.

Copy link
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

Definitely on the right track. Lets go back and forth in the discussion

@bparks13
Copy link
Member Author

Below is a distilled list of the comments that exist above, given as verbal feedback during our issue sorting meeting. Need to go through and make sure that all comments above are resolved, even if they do not appear in this list.

  • Rename the automated ProbeInterface file to something more reasonable
    • Remove headstage from the file name (Question: should we keep “oni” or not?)
    • Note that this might actually be kind of difficult, since the headstage is embedded in the DeviceName property as a full string containing headstageName/deviceName. In MultiDeviceFactory.GetFullDeviceName(), we have hard-coded the character / as the separator, but this does not guarantee that this separator won't change in the future, and it also doesn't guarantee that the user does not include their own / character in the device name.
  • The ProbeInterfaceFileName should be nested inside of the configuration, at the same level as the calibration filenames
  • Incoherence between the ProbeInterface and Probe Group names, should be streamlined
    • For clarity, call everything Probe Group. Probe Interface is the specification, but probe group is what we are manipulating.
  • Update the Gain/Adc calibration variable names to say “FileName” instead of just "File"
  • Add note to the XML comments on why we are not using the Obsolete attributes
    • Point to the existing issue on why we can’t do it, since XML serializer ignores obsolete attributes

@bparks13
Copy link
Member Author

@jonnew Here are my responses to your review:

Change the filename

  • After updating it based on your recommendation, this is how all of the current filenames will be displayed for users. The headstage has been removed, and the device name remains (grabbed using DeviceType.Name). There are duplicated addresses here, but the format will be the same regardless:
image

Display default filename grayed out

  • Looking into the PropertyGrid, it does not seem to be very easy to update the visualization (i.e., grayed out text) unless we create a new editor that overrides UITypeEditor.
  • What can be done easily is add a tag marking it as the default (e.g., [default]). It would be a bit more involved to display the actual filename that would be saved, since the NeuropixelsV2 devices have two devices per address, and therefore require additional context to create the correct filename, which is not easily accessed in the TypeConverter.
    • To be more specific, in the TypeConverter I can access the current filename object as a string, but I do not know if it is the ProbeA or ProbeB object without using reflection or some other checking method
    • The other devices are easy, since they can call ProbeGroupHelper.GenerateProbeInterfaceFilename(DeviceAddress, DeviceType.Name) and get the correct name, but then there needs to be some boilerplate added to check which device is currently being modified (we have this information with the ITypeDescriptorContext context input argument, so this is a possibility)
  • Here is what the default tag would look like:
image

Incoherence in Naming/Loading

I agree with points 1/2 in principle, the probe interface filename should be a part of the probe configuration, as it is theoretically on the same level as the calibration files.

  • However, this is not directly possible to move the filename into the probe configuration. Based on the section above where we are talking about the file name pattern, I need to know the DeviceType.Name and DeviceAddress to create the correct filename, but if I move the property into the configuration class then I lose access to these properties.
  • If we have an easy way to link the configuration class back to the device we could do that, but I'm worried that going down that route will make logic convoluted to keep things consistent, and I don't want to have multiple places to update state (such as by duplicating the device address as a field in the configuration class).

For point 3, I’ll make the names consistent. To clarify: the GUI’s load/save menu updates the electrode mapping currently in memory, while the FileName property determines where the probe configuration is saved with the Bonsai workflow. Both use the ProbeInterface specification to load/save files, but the purposes differ; GUI load/save modifies the active mapping for runtime, whereas FileName sets the location of the saved probe representation.

Other Miscellaneous Comments

Updated the XML comments with a link to the issue that prevents us from tagging certain properties as [Obsolete], cleaned up XML docs, removed unnecessary ones, and conformed to standard naming conventions.

Also, converted the existing Gain/Adc calibration files to have FileName at the end, for consistency across Bonsai nodes and with the ProbeGroupFileName we are adding here.

Outstanding Questions

  1. Should we add a default tag to the ProbeGroupFileName property? Or leave it blank?
  2. Should we leave the ProbeGroupFileName property at the level of the device? Or move it to the Configuration class and figure out how to access the data needed to set the filename properly?

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.

ProbeConfiguration is deserializing ChannelConfiguration to XML
2 participants