Add documentation to handle (physical) units of recording channels #3844
Conversation
h-mayorquin
added
documentation
|
Looks great - doc was super clear and easy to read! |
Co-authored-by: Chris Halcrow <57948917+chrishalcrow@users.noreply.github.com>
There was a problem hiding this comment.
Before being really careful my one concern with this doc as written is that for the average user we don't need to scale_to_uV. It happens with argument flags in various functions. The way this documentation works it sounds like I always have to create a scaled recording rather than keep my unscaled and scale at the function level. I think the doc about other physical units is great, but I'm worried that this might actually confuse users on how to use the library since the scaling for uV is baked in to other functions the class is for people that just need their recording scaled for other purposes right?
doc/how_to/physical_units.rst
Outdated
| @@ -0,0 +1,96 @@ | |||
| Working with physical units in SpikeInterface recordings | |||
| =============================================== | |||
There was a problem hiding this comment.
rendering is impossible for the suggestion but this needs to go to the end of the heading.
There was a problem hiding this comment.
Generated by copilot before.
Fixed.
doc/how_to/physical_units.rst
Outdated
|
|
||
|
|
||
| Converting to Physical Units | ||
| ------------------------- |
There was a problem hiding this comment.
Thanks, fixed.
what would be the confusion exactly? |
|
The confusion is do I need to do a recording = read_intan()
scaled_rec = scale_to_uV(recording)
sorting = run_sorter(xx)
# or spikeglx
recording = read_spikeglx()
scaled_rec= scale_to_uV(recording)
sorting=run_sorter()The scaling is not required in a pipeline. But when I read these docs I feel like I have to explicitly run this step no matter what. I think it is helpful for people to know scaling is important but the main way the library has people interact with ephys scaling is with the |
Thanks.
This is not clear to me either. Is it to you?
I don't agree with this or want it. Yes, I think we should discuss this on the meeting tomorrow. I am fine if you want add a list of pipes/algorithms where the units don't matter and an explanation on why they don't but I don't have that knowledge and I users are in a better position to understand if they want to run their algorithms on raw data or in units. |
|
@yger it seems like maybe some changes to SC2 broke testing? |
|
Ok @zm711 I added some notes in the direction that you wanted. Check it out and let me know what you think. I still think that this is a good topic to discuss on the next meeting. |
doc/how_to/physical_units.rst
Outdated
| SpikeInterface provides tools to handle both situations. | ||
|
|
||
| It's important to note that **most spike sorters work fine on raw digital (ADC) units** and not scaling is needed. | ||
| Many preprocessing tools are also linear transformations, and if the ADC is implemented as a linear transformation, the overall effect can be preserved. |
There was a problem hiding this comment.
we could say that is relatively common here to be a linear transformation (although you discuss gain/offset later on).
There was a problem hiding this comment.
I added a comment on this direction.
| Therefore, **it is usually safe to work in raw ADC units unless a specific tool or analysis requires physical units**. | ||
| If you are interested in visualizations, comparability across devices, or outputs with interpretable physical scales (e.g., microvolts), converting to physical units is recommended. | ||
| Otherwise, remaining in raw units can simplify processing and preserve performance. | ||
|
|
There was a problem hiding this comment.
We might want Alessio or Sam to comment on some of the internal tooling. I think the scaling is automatic for some of our stuff so we should make it clear if/when we do that. I just don't remember off the top of my head. If Alessio doesn't have the time to look this over I can doublecheck in the code.
There was a problem hiding this comment.
I think it is better if you check it.
There was a problem hiding this comment.
I think this is only missing part to move this forward?
There was a problem hiding this comment.
Yeah I agree @alejoe91 could you comment here when you have a moment :)
|
|
||
| physical_value = raw_value * gain + offset | ||
|
|
||
|
|
There was a problem hiding this comment.
Maybe a note here saying that as we discussed above because this is a linear transformation we can do preprocessing etc without an issue.
There was a problem hiding this comment.
Can you add a suggestion?
There was a problem hiding this comment.
Yep will do. Have some experiments all day today, but my hope is tomorrow should be a little freer.
doc/how_to/physical_units.rst
Outdated
| values = ["volts"] * num_channels | ||
| recording.set_property(key='physical_unit', value=values) | ||
|
|
||
| values = [0.001] * num_channels # Convert from ADC to volts |
There was a problem hiding this comment.
Maybe we say
gain_values = [0.001] * num_channels
to be even more clear and then below we would say
offset_values
just so we aren't using the same variable being overwritten for both? This is definitely optional.
There was a problem hiding this comment.
Yeah, makes sense.
|
|
||
| def __init__(self, recording): | ||
| if "gain_to_physical_unit" not in recording.get_property_keys(): | ||
| raise ValueError("Recording must have 'gain_to_physical_unit' property to convert to physical units") |
There was a problem hiding this comment.
Any interest in adding the way to do this in the error? for example. adding a line like
please use the set_property function in order to set "gain_to_physical_unit"
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
| self.set_channel_offsets(offsets=0.0) | ||
|
|
||
|
|
||
| scale_to_physical_units = ScaleToPhysicalUnits |
There was a problem hiding this comment.
Hello, could you change this to
from spikeinterface.core.core_tools import define_function_handling_dict_from_class
scale_to_physical_units = define_function_handling_dict_from_class(ScaleToPhysicalUnits, name="scale_to_physical_units")then will works with dicts of recs
| if "gain_to_physical_unit" not in recording.get_property_keys(): | ||
| error_msg = ( | ||
| "Recording must have 'gain_to_physical_unit' property to convert to physical units. \n" | ||
| "Set the gain using `recording.set_property(key='gain_to_physical_unit', value=values)`." |
There was a problem hiding this comment.
| "Set the gain using `recording.set_property(key='gain_to_physical_unit', value=values)`." | |
| "Set the gain using `recording.set_property(key='gain_to_physical_unit', values=values)`." |
| if "offset_to_physical_unit" not in recording.get_property_keys(): | ||
| error_msg = ( | ||
| "Recording must have 'offset_to_physical_unit' property to convert to physical units. \n" | ||
| "Set the offset using `recording.set_property(key='offset_to_physical_unit', value=values)`." |
There was a problem hiding this comment.
| "Set the offset using `recording.set_property(key='offset_to_physical_unit', value=values)`." | |
| "Set the offset using `recording.set_property(key='offset_to_physical_unit', values=values)`." |
Ok guys, I will need your feedback. I am also adding another preprocessor wrapper that handles this for the user.