Add documentation to handle (physical) units of recording channels

[h-mayorquin]

Ok guys, I will need your feedback. I am also adding another preprocessor wrapper that handles this for the user.

[chrishalcrow]

Looks great - doc was super clear and easy to read!

[zm711]

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?

[h-mayorquin]

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

what would be the confusion exactly?

[zm711]

The confusion is do I need to do a scale_to_uV step in all of my pipelines? ie

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 return_scaled soon to be return_in_uV (or whatever we decided) as a function argument. Not as a preprocessing step. Does that make sense? I think the docs are good, but I'm worried people will not realize when they should scale outside of function calls vs inside. For another example. If I take a scale_to_uV recording what happens if I try to return_scaled? I shouldn't rescale again (I think you may have added an error, but then that mechanism seems weird. Because data should be scaled. Maybe this is worth an actual discussion because I'm worried I'm not being clear.

[h-mayorquin]

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.

Thanks.
We can add a comment that scaling is not needed in sorters.

I think the docs are good, but I'm worried people will not realize when they should scale outside of function calls vs inside.

This is not clear to me either. Is it to you?

but the main way the library has people interact with ephys scaling is with the return_scaled soon to be return_in_uV

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.

[zm711]

@yger it seems like maybe some changes to SC2 broke testing?

[h-mayorquin]

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.

[zm711]

Few more comments.

[zm711]

we could say that is relatively common here to be a linear transformation (although you discuss gain/offset later on).

[h-mayorquin]

I added a comment on this direction.

[chrishalcrow]

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

[h-mayorquin]

Done

This function define_function_handling_dict_from_class needs way more documentation of what it has now.

[zm711]

I have reread this I request two minor changes and then I feel good about this.

[h-mayorquin]

OK, I implemented changed based on the last round of review @zm711 .

[zm711]

Two grammar fixes :)

still slight preference for a mentioned of return_scaled but after merging the grammar fixes I'm fine with approving and then if we have user issues/confusion we can adjust to what they need.

[h-mayorquin]

Two grammar fixes :)

still slight preference for a mentioned of return_scaled but after merging the grammar fixes I'm fine with approving and then if we have user issues/confusion we can adjust to what they need.

I don't want to mention this until #3794 is done.

[zm711]

I don't want to mention this until #3794 is done.

Yeah I forgot about that until you pushed another commit :)

That works for me!