Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keep same dtype after scale_to_uV #3660

Closed
wants to merge 1 commit into from

Conversation

RobertoDF
Copy link
Contributor

@RobertoDF RobertoDF commented Jan 30, 2025

Right now rec type gets converted silently when applying scale_to_uV. This commit preserves dtype.

@zm711
Copy link
Collaborator

zm711 commented Jan 30, 2025

I think the issue with this is if your recording has a dtype of int then you could either overflow or truncate your actual result. There have been debates about--I think @h-mayorquin would be good to comment on this. But I think in general if you're scaling the dtype should be a float. But maybe instead of float32 we should just make the default be float to allow for float64 if necessary.

What is the use case you're thinking of?

@alejoe91
Copy link
Member

not sure about this. IMO microvolts are float by definition, since it's a physical unit. So it makes sense that you would need another astype if you want to coerce to ints

@RobertoDF
Copy link
Contributor Author

I see, I have a recording with ONIX and these binaries are np.uint16 so this hidden conversion was causing me problems. So I should first convert to float and afterward scale?

@RobertoDF
Copy link
Contributor Author

Right now in their official docs they propose:

ap_scalar = 1.2e6 / (2 ** bit_depth) / ap_gain
ap_offset = (2 ** (bit_depth - 1)) * ap_scalar
ap_recording = se.read_binary(os.path.join(data_directory,  f"np1-spike_{suffix}.raw"), 
                           3e5, 
                           np.uint16, 
                           num_channels, 
                           gain_to_uV=ap_scalar, 
                           offset_to_uV=-ap_offset) 

@zm711
Copy link
Collaborator

zm711 commented Jan 30, 2025

But in that case you would just be storing the gain and offset in the recording object. Then if you wanted to scale to voltages we would change from the uint16 to floats. I agree with Alessio, the voltages are real units with decimals so float makes the most sense for voltage. Kilosort's recommendation is just to truncate to int16, but that would be after the conversion of the voltages (so you're losing precision to have better data management). So that's basically what Alessio is saying. Scale to float (because we are going to voltages and then astype if you want to go back to an int).

@RobertoDF RobertoDF closed this Jan 30, 2025
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