Skip to content

Conversation

@khl02007
Copy link
Contributor

@khl02007 khl02007 commented Sep 5, 2025

  • By default saves the data in microvolts, as it has been done so far
  • If there is save_as_microvolts field in metadata, uses that
  • Saves the right conversion factor

@codecov
Copy link

codecov bot commented Sep 5, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.43%. Comparing base (9541a17) to head (4947df7).

Files with missing lines Patch % Lines
src/trodes_to_nwb/convert_ephys.py 75.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
- Coverage   89.53%   89.43%   -0.11%     
==========================================
  Files          13       13              
  Lines        1711     1722      +11     
==========================================
+ Hits         1532     1540       +8     
- Misses        179      182       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@edeno
Copy link
Collaborator

edeno commented Sep 5, 2025

I am still not convinced this is best to have in general for the lab. We would like to keep data processed as similarly as possible and this would lead to a divergence that, as far as I know, only you are interested in. If there is a truly good reason to save it in ADC units, we should do it universally, but I would want to know the pros and cons of such.

@samuelbray32
Copy link
Collaborator

If I remember correctly from a lab meeting when we were writing this package, we determined that the precision lost from converting is less than the noise in the recording. I agree that unless we have a scientific reason for it, having a mix of units between lab files is unnecessary confusion.

@lfrank
Copy link

lfrank commented Sep 5, 2025 via email

@khl02007
Copy link
Contributor Author

khl02007 commented Sep 5, 2025

  • If this package is to be used by people outside of our lab, I'm pretty sure there are many who would be interested in this feature, because this is the recommended way of doing NWB conversion: keep the data values and conversion factor separate. You can find many examples of this in DANDI. In my own lab (if I ever get to have one in the future) I plan to save the data in ADC units and I'd like to be able to use this package.
  • Current practice adds 0.5 microV of noise on average to each sample. This might be small, but it seems strange to add even a small amount of noise to data unnecessarily. Unless we have a scientific reason for it, we shouldn't do this.
  • We're not introducing new units here. Converting to physically meaningful unit is a simple matter of multiplying by the conversion factor.
  • I'm not saying we should do this for all Frank lab data going forward. Note that this feature is optional. Even if we do make this change for Frank lab data going forward, this won't lead to any divergence. We just have to multiply the electrical series we load by the conversion factor. We probably should do this anyway, since we may want to process NWB files from outside Frank lab. I'm happy to add that line wherever the electrical series is loaded in Spyglass (this happens only for LFP and spike sorting pipelines, and spike sorting pipeline already does this).

@lfrank
Copy link

lfrank commented Sep 5, 2025 via email

@samuelbray32
Copy link
Collaborator

To confirm, keeping our data consistent means this option wouldn't be used for things in the franklab database correct?

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.

4 participants