Add _roi_response_denoised to store denoised traces

[alessandratrapani]

Denoised calcium traces should be distinguished from raw calcium traces.
See discussion in #287 (comment)

This implementation should also fix #249.

TODO

• Add background spatial components
• Add raw traces as C+Yr
• Add testing

[EricThomson]

Hi I'm just in here poking around your PR 😄 It looks great 🚀

I think you probably want f here for neuropil for the trace_extractor() method (that's the background fluctuations). b is the background spatial component(s) that would correspond for instance to A for the neural components. Forgive me if I've misunderstood how things work I'm pretty new to NWB.

I'm thinking it's just a typo (our variable names are awful, b/f what do those mean? 😆 ), as f is what you noted in the original issue.

[alessandratrapani]

Thank you so much @EricThomson. I am now trying include the spatial component "b" as well.

[pauladkisson]

For neuroconv #783, I need

• get_background_ids() following the example of get_roi_ids
• get_background_image_masks() following the example of get_roi_image_masks()
• get_background_pixel_masks() following the example of get_roi_pixels_masks()

[codecov]

Codecov Report

Attention: Patch coverage is 71.87500% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 79.25%. Comparing base (94691a4) to head (7ad05f5).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
- Coverage   79.35%   79.25%   -0.10%     
==========================================
  Files          39       39              
  Lines        3071     3100      +29     
==========================================
+ Hits         2437     2457      +20     
- Misses        634      643       +9     

Flag	Coverage Δ
unittests	79.25% <71.87%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...s/extractors/caiman/caimansegmentationextractor.py	86.77% <100.00%> (+1.59%)	⬆️
src/roiextractors/testing.py	99.33% <100.00%> (ø)
src/roiextractors/segmentationextractor.py	86.82% <43.75%> (-4.57%)	⬇️

[pauladkisson]

Looks good on first viewing -- I'll try it out in neuroconv and see if I run into any issues.

[pauladkisson]

I noticed that CaimanSegmentationExtractor doesn't have any dedicated unit tests -- maybe this would be a good time to put that together?

I'm still pretty torn about whether we should go for a rolling refactor or an intensive refactor for the testing. @CodyCBakerPhD maybe you can weigh in?

Also, perhaps it is time to add a deprecation warning to the write_segmentation method since we are moving the api away from write support.

[EricThomson]

Note away on vacation until end of next week but I should be able to start looking more then. Very exciting stuff!

[CodyCBakerPhD]

I noticed that CaimanSegmentationExtractor doesn't have any dedicated unit tests -- maybe this would be a good time to put that together?

Looks like it's a part of the general roundtrip tests: https://github.com/catalystneuro/roiextractors/blob/main/tests/test_io.py

From that perspective, dedicated tests would only be for specific assertions of specific structures unique to Caiman that can't be easily included as params in the generic testing framework

I'm still pretty torn about whether we should go for a rolling refactor or an intensive refactor for the testing. @CodyCBakerPhD maybe you can weigh in?

Rolling in small iterative steps is always appreciated for review - for tests as well, there's nothing wrong with keeping the current infrastructure, adding a new one, then remove the old one after the new one is completely done

I'd be in favor of a refactor to something akin to the mixin style of NeuroConv interface tests, which allow you to define format-specific tests pretty easily

Also, perhaps it is time to add a deprecation warning to the write_segmentation method since we are moving the api away from write support.

Yeah, I share the philosophy of SpikeInterface that supporting format-specific writing methods is beyond our scope in this package

[pauladkisson]

Works well in neuroconv, I think this is good to go.