Correct 'global' and 'local_fit' background corrections #139
Conversation
JSON convert python tuples to lists. This caused a bug where a tuple was compared to a list which resulted in "False" when the result should have been "True". Specifically, this bug caused the `utils.bg_read` function to average over an ROI area when it shouldn't, which in turn made recOrder's BG correction incorrect (or at least different from expected).
`waveorder`'s `Polscope_bg_correction` function requires a background to be supplied, and this background will be subtracted for all `bg_option` choices ('global', 'local_fit', and 'none').
Before this commit, `local_fit` background correction was applying a background correction from file (which might be empty if the GUI wasn't present, or it might be a real file if read from a config file) before proceeding to the local_fit correction.
After this commit, the 'local_fit' option is applying an "identity correction" (i.e. no correction) then proceeding to apply the local_fit correction.
…ion works Applying a 2D background correction to 3D data in `waveorder` depends on `self.N_decocus` being set correctly. `recOrder`'s branching logic did not set these parameters correctly, and this commit fixes the issue.
|
Preview page for your plugin is ready here: |
recOrder/pipelines/qlipp_pipeline.py
Outdated
| if self.config.background_correction == 'global': | ||
| print("Loading bg from "+self.bg_path) | ||
| bg_data = load_bg(self.bg_path, self.img_dim[0], self.img_dim[1], self.bg_roi) | ||
| self.bg_stokes = self.reconstructor.Stokes_recon(bg_data) | ||
| self.bg_stokes = self.reconstructor.Stokes_transform(self.bg_stokes) | ||
|
|
||
| elif self.config.background_correction == 'local_fit': | ||
| self.bg_stokes = np.zeros((5, self.img_dim[0], self.img_dim[1])) | ||
| self.bg_stokes[0, ...] = 1 # Set background to "identity" Stokes parameters. waveorder's 'local_fit' applies this identity correction. | ||
| else: | ||
| self.bg_stokes = None | ||
|
|
There was a problem hiding this comment.
Thanks for debugging this @talonchandler!
In the most common mode of reconstruction, we first apply the measured background (aka 'global' correction) and if needed we further fit and subtract a surface from this once already background corrected data. This is what I call "local" background correction, but we've seen that these names are poor. In practice it's a combination of "global" correction plus local fit.
The "global" background correction should simply subtract the background images, as is implemented here.
In this implementation, "local" background correction only subtracts the fitted 2d surface. I think it's better to rename this method and apply what I described at the top.
There are cases where we cannon acquire a background image and perform "global" background correction. In this case, we would only do surface fitting. You've implemented this here as providing unity background images.
So I think we need a total of four background correction methods:
- "none" - no background correction
- "measured" - use only background images
- "measured+" - use background images and surface fit to remove any residual background
- "estimated" - only use surface fit
There was a problem hiding this comment.
Aha! This context really helps me see why the waveorder code looks the way it does. I 100% agree with needing four options (either through a single four-option menu, two two-option menus, or two checkboxes).
I was originally thinking of putting the "renaming of options" into it's own PR, but with this comment I think now I'll do my renaming here.
|
I've implemented a first-pass at what Ivan suggested with the following user exposed names and tooltips: None: No background correction
Here are the results on the test data. Although the "Estimated" and "Measured + Estimated" look similar, they are subtly different...the "Estimated" correction just seems to dominate for this example. Does this match your experience for samples like this, Ivan? I don't have much experience with the fitting to judge whether or not this is working well (although I'm tracing the code and seeing it hit the "right places").
I will test on hummingbird on Monday, implement bug fixes, then I'll mark as ready for review. In the meantime, comments are welcome on my naming choices (@Soorya19Pradeep) and the results from the test data. |
|
I forgot to mention that these new names are only in the GUI because I need to maintain backwards compatibility. I left the old names unchanged in the config files and added one new option |
Yes, this looks right to me. On a sample like this where you have large correlation in S1 and S2 over the field of view the right thing to do is to apply measured background correction, rather than a local fit. Local fit works well where structures in the sample are isotropically distributed on the scale of the field of view (think of cultured cells). When S1 and S2 have large correlations (imagine a FOV with sarcomeres all running in the same direction) then the local fit will try to eliminate those, which will be wrong. In the Kazansky dataset here, the 2D surface fit is affected by the structures of the target and you see larger background than just using the measured background. Note that your perception of random orientation may be skewed here by what colors we perceive as bright. |
|
Suggestion for rewording the tooltips. Method names are still up for discussion, I think. None: No background correction |
|
Thanks for iterating on the tooltips, Ivan. My latest iteration of names and tooltips (with my minor changes...all open for discussion) are: None: No background correction. Here are the background corrections applied to a background image with histograms:
No major surprises from my perspective. I also had a session on hummingbird where I tested the background correction behavior in "Online" and "Offline" mode...no surprises. I'm changing from draft to PR now, since I think this is ready for review+merge. All comments appreciated! |
|
Thanks @talonchandler for these fixes and @ieivanov for the review. The refined terminology (measured, estimated, measured + estimated) reads well. A suggestion for a tooltip for It looks like this PR takes care of issues #104 and #136. I've noted recent ideas for improving background correction on an older issue (#4). This PR has also has useful notes on how waveOrder's background correction can be improved. I think the PR is ready to merge. |
|
Thanks for reading + rephrasing, Shalin, and thanks for bringing up #4. I'll let the tests run and wait for Ivan's last word before merging. |
|
Sorry, I'm behind on this. Looks good to me too, thanks for merging. |
recOrder's background correction was not behaving in the way that I expected---see attached figure. I tested recOrder's GUI using this test-config.yml on the zenodo data.'local' and 'global' corrections were giving identical results while 'none' seemed to work (see first row).
Three simultaneous fixes were required:
recOrderalone, I needed to use a kludge to conform towaveorder'sPolscope_bg_correctionfunction. Specifically,Polscope_bg_correctionrequires and applies an array-based background correction no matter what background correction option you choose. To work around this for thelocaloption, I supplied an "identity" background correction.AFAIK most users have been avoiding
Polscope_bg_correctionby applying their corrections "by hand" in scripts. I think myself, @Soorya19Pradeep, and @JohannaRahm are doing this based on scripts I've seen. I think this is a strong argument for either rewritingPolscope_bg_correctionor refactoring this functionality, but here I'm working under the constraint of a fixedwaveorderdependency.Details (skippable): This bug was caused by JSON converting python tuples to lists. A tuple was compared to a list which resulted in "False" when the result should have been "True". Specifically, the
utils.bg_readfunction averaged over an ROI area when it shouldn't, which in turn made recOrder's BG correction incorrect (or at least different from expected). This means that both 'local' and 'global' corrections were behaving incorrectly.The
local_fitlogic inwaveorderdepends onself.N_defocus(L1181waveorder_reconstructor.py). My first choice would be to improvewaveordersince the background correction code should have close to zero dependencies on the constructor object...but I'm only applying fixes torecOrderhere so I supplied more options to thewaveorderconstructor so thatself.N_defocusis correct.The result is matching my expectations (see second row of figure), though I'd appreciate input from @ieivanov and @mattersoflight. I will test on hummingbird before merging.
I am fine with these fixes for 0.2.0, but I would like to consider cleaning up
waveorderfor 1.0.0 because this was a challenging bug to fix and I expect the background correction code to continue to cause headaches.