Skip to content

fix(connectivity): robust normalization; clamp coherence to [0,1]#62

Merged
edeno merged 4 commits into
masterfrom
fix/coherence-clamp-stability
Oct 17, 2025
Merged

fix(connectivity): robust normalization; clamp coherence to [0,1]#62
edeno merged 4 commits into
masterfrom
fix/coherence-clamp-stability

Conversation

@edeno

@edeno edeno commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

Summary

  • Replace zero-clamp with epsilon-clamp for denominators to prevent numerical instability
  • Clip coherence magnitude and imaginary coherence to mathematically valid [0,1] range
  • Add comprehensive test suite for bounds verification with edge cases

Changes

  • coherency(): Use xp.maximum(norm, xp.finfo().eps) instead of norm[norm == 0] = xp.nan
  • coherence_magnitude(): Add xp.clip(magnitude, 0, 1) to ensure valid bounds
  • imaginary_coherence(): Add epsilon clamp + [0,1] clipping for numerical stability
  • New tests: Verify bounds with random data, edge cases, and zero-power scenarios

Risks & Mitigations

  • Low risk: Mathematical constraints enforced, no API changes
  • Tests confirm no new failures introduced

Test plan

  • 3 new tests pass (bounds checking, edge cases, zero power)
  • All existing tests maintain same status
  • Coherence values properly bounded in [0,1] range

🤖 Generated with Claude Code

- Replace zero-clamp with epsilon-clamp for denominators to prevent division by zero
- Clip coherence magnitude and imaginary coherence to valid [0,1] range
- Add comprehensive tests verifying bounds with edge cases
- Handle numerical instability in normalization paths gracefully

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coveralls

coveralls commented Sep 24, 2025

Copy link
Copy Markdown

Coverage Status

coverage: 74.35% (+0.1%) from 74.234%
when pulling bb4baca on fix/coherence-clamp-stability
into 6f6c095 on master.

- Remove unused pytest import from test_coherence_bounds.py
- Apply black formatting to modified files
- Resolve F401 flake8 error

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@edeno edeno requested a review from Copilot September 24, 2025 17:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves numerical stability in coherency calculations by implementing robust normalization and mathematical bounds enforcement. It replaces zero-clamp operations with epsilon-clamping to prevent division by zero, and clips coherence values to their valid [0,1] range to ensure mathematically correct outputs.

  • Replace zero-clamp with epsilon-clamp for denominators to prevent numerical instability
  • Add bounds clipping for coherence magnitude and imaginary coherence to [0,1] range
  • Include comprehensive test suite to verify proper bounds handling with edge cases

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

File Description
spectral_connectivity/connectivity.py Core coherency calculations with epsilon-clamping and bounds clipping
tests/test_coherence_bounds.py New comprehensive test suite for bounds verification
examples/*.ipynb Whitespace cleanup removing trailing newlines

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

self._power[..., :, xp.newaxis] * self._power[..., xp.newaxis, :]
)
norm[norm == 0] = xp.nan
norm = xp.maximum(norm, xp.finfo(norm.dtype).eps)

Copilot AI Sep 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using xp.finfo(norm.dtype).eps repeatedly could be inefficient. Consider caching this value once since norm.dtype is consistent within the method.

Suggested change
norm = xp.maximum(norm, xp.finfo(norm.dtype).eps)
eps = xp.finfo(norm.dtype).eps
norm = xp.maximum(norm, eps)

Copilot uses AI. Check for mistakes.
denominator = xp.sqrt(
self._power[..., :, xp.newaxis] * self._power[..., xp.newaxis, :]
)
denominator = xp.maximum(denominator, xp.finfo(denominator.dtype).eps)

Copilot AI Sep 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the coherency method, xp.finfo(denominator.dtype).eps could be cached to avoid repeated computation since the dtype remains constant.

Suggested change
denominator = xp.maximum(denominator, xp.finfo(denominator.dtype).eps)
eps = xp.finfo(denominator.dtype).eps
denominator = xp.maximum(denominator, eps)

Copilot uses AI. Check for mistakes.
).astype(np.complex128)

# Add very small values that could cause numerical issues
fourier_coefficients[0, 0, 0, 0, :] = 1e-15

Copilot AI Sep 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 1e-15 should be defined as a named constant (e.g., SMALL_VALUE = 1e-15) to improve code readability and maintainability.

Copilot uses AI. Check for mistakes.
).astype(np.complex128)

# Add edge case with very small power
fourier_coefficients[0, 0, 0, 0, :] = 1e-16

Copilot AI Sep 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 1e-16 should be defined as a named constant to improve code readability and maintainability, similar to the 1e-15 value used elsewhere.

Copilot uses AI. Check for mistakes.
edeno and others added 2 commits October 17, 2025 14:40
Merged latest master branch changes. Notebook conflict resolved by taking master version.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Document epsilon-clamping and bounds clipping changes in PR #62.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@edeno edeno merged commit b78f5a9 into master Oct 17, 2025
4 of 9 checks passed
@edeno edeno deleted the fix/coherence-clamp-stability branch October 17, 2025 18:52
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