fix(metrics): mean-center before Hamming window in FRC/FSC preprocessing#42
Merged
Merged
Conversation
preprocess_images was applying the Hamming taper directly to the raw image. With a large DC offset μ the taper produces a μ·(w(x)−mean(w)) low-frequency artifact that survives the later image−image.mean() DC removal (zero spatial mean, non-zero power across low-frequency bins), shifting the FSC=0.143 crossing on high-offset data. Mirrors what DCR's _preprocess_dcr_image already does (mean-subtract, then window). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts FRC/FSC preprocessing so that images are mean-centered before applying the Hamming window, eliminating a large low-frequency artifact for high-DC-offset inputs while keeping the existing defensive post-window DC subtraction. Flow diagram for updated FRC/FSC image preprocessingflowchart TD
A[preprocess_images start] --> B[image1,image2 input]
B --> C{zero_padding enabled?}
C -->|yes| D[pad_image_to_cube image1]
D --> E[pad_image_to_cube image2]
C -->|no| E
E --> F{disable_hamming?}
F -->|yes| G[skip Hamming window]
F -->|no| H["image1_minus_mean = image1 - image1.mean()"]
H --> I["image2_minus_mean = image2 - image2.mean()"]
I --> J[image1 = hamming_window image1_minus_mean]
J --> K[image2 = hamming_window image2_minus_mean]
G --> L[return preprocessed images]
K --> L
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="cubic/metrics/spectral/frc.py" line_range="153-154" />
<code_context>
if image2 is None:
raise RuntimeError("image2 must be set after splitting")
- image2 = hamming_window(image2)
+ image1 = hamming_window(image1 - image1.mean())
+ image2 = hamming_window(image2 - image2.mean())
if image2 is None:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Mean subtraction will propagate NaNs across the entire image, which may be undesirable compared to the previous behavior.
Because the mean is now computed over the full array, any NaNs in `image1`/`image2` will make `image*.mean()` NaN, and thus `image* - image*.mean()` will turn the entire array into NaNs instead of keeping them localized. If NaNs are expected (e.g., masked pixels), consider `np.nanmean` or masking so missing data doesn’t corrupt the whole windowed image.
Suggested implementation:
```python
# Use NaN-aware means so missing data does not corrupt the whole image
image1 = hamming_window(image1 - np.nanmean(image1))
image2 = hamming_window(image2 - np.nanmean(image2))
```
1. Ensure this file imports numpy as `np` at the top if it does not already:
- `import numpy as np`
2. If your codebase prefers explicit masking instead of `nanmean`, you could instead:
- Compute a mask for valid pixels (e.g. `valid1 = np.isfinite(image1)`) and compute the mean only over valid pixels.
- Apply the mean subtraction only where the mask is true, leaving invalid pixels unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR updates FRC/FSC preprocessing so split images are mean-centered before Hamming windowing, reducing low-frequency artifacts caused by tapering high-DC-offset inputs.
Changes:
- Mean-subtracts each preprocessed split image before applying
hamming_window. - Adds an explanatory comment documenting why centering must happen before tapering.
- Keeps downstream defensive DC removal unchanged.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…on tests Moves mean-subtraction ahead of pad_image_to_cube so the zero-padded ring stays at zero instead of carrying a -diluted_mean offset that the Hamming taper would leak back into low-frequency bins. On a non-cubic shape with DC offset, low-frequency artifact power drops a further ~600x beyond the initial fix and now matches a centered-input reference. Binomial counts splitting still needs raw photon counts, so its halves are centered after splitting instead. Adds two regression tests pinning both ordering invariants (center-before-window, center-before-pad) by comparing low-frequency FFT power against a DC-only-shifted reference (Copilot review on PR #42). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alxndrkalinin
added a commit
that referenced
this pull request
Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
preprocess_images(cubic/metrics/spectral/frc.py) applied the Hamming taper directly to the raw image. The Hamming window has mean ~0.54 and goes to zero at edges; multiplying an image with mean μ by this taper produces aμ·(w(x) − mean(w))low-frequency artifact that survives the laterimage − image.mean()DC removal in the FFT layer — the artifact has zero spatial mean by construction but non-zero power across all low-frequency bins.On a membrane-fluorescence-like input (mean ≈ 2500, range 800–14600) the artifact carries 761× more low-frequency power than the corrected pipeline, shifting the FSC=0.143 crossing.
Fix
Mean-subtract each (already-split) image before windowing in
preprocess_images. Mirrors whatdcr.py:_preprocess_dcr_imagealready does correctly (center, then Tukey). The post-windowimage − image.mean()stays as defensive DC removal — a windowed zero-mean signal still has a tiny residual mean from the non-uniform weighting.After the fix, low-frequency power on the same synthetic input drops from
2.7e16→2.1e12(a ~4-order-of-magnitude reduction).Impact
Touches every FRC/FSC path routed through
preprocess_images:FRC(2D)DirectionalFSC(3D, mask backend)_calculate_frc_corehist backend_calculate_fsc_sectioned_histDCR is unaffected — it already centered before windowing.
Resolution estimates on high-DC-offset data will shift (should improve, not regress). No change expected on already-centered or low-offset inputs.
Test plan
pytest tests/metrics/spectral/— 27 passed, 4 GPU-skippedruff check+ruff format --checkcleanmypy --ignore-missing-imports cubic/clean (via pre-commit hook)🤖 Generated with Claude Code
Summary by Sourcery
Bug Fixes: