Skip to content

Fix RotD calculation - #215

Merged
lispandfound merged 5 commits into
masterfrom
rotd_calculation_fix
Jul 24, 2026
Merged

Fix RotD calculation#215
lispandfound merged 5 commits into
masterfrom
rotd_calculation_fix

Conversation

@lispandfound

@lispandfound lispandfound commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Found due to work from Alan. The old code essentially calculated the wrong side of the median in the RotD50 calculation, resulting in RotD50 being biased high. The fix is a small one to use bog-standard sorting to calculate median, which is not only simpler but more correct. I have also added a bunch more rotd calculation tests, including hypothesis tests that assert:

  1. rotd100 / rotd50 remains within floating point epsilon of sqrt(2) (guarantee is 1e-12),
  2. For a special class of records (linearly polarised records), the sqrt(2) bound is precisely attained,
  3. For all pairs of rotd_0 and rotd_90 the parallel codepath is exactly identical to the single-threaded codepath. I plan to remove the parallel code at a later date but in the meantime this is an important assertion.

By "hypothesis test" I mean testing in the form of the hypothesis library: automatically testing a wide variety of cases and then crucially shrinking to the simplest possible failure case.

Line count is high because I bumped the uv and cargo lock files. Real pull request is +300ish lines, mostly tests and some minor ruff fixes.

@ucgmsim ucgmsim deleted a comment from gemini-code-assist Bot Jul 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes the Rust RotD50 (median) computation to avoid biased-high results by switching to a fully sorted median selection, and strengthens correctness guarantees with additional property-based RotD tests. The PR also includes several small Python-side typing/lint cleanups plus dependency / lockfile updates.

Changes:

  • Correct RotD50 calculation in src-rust/rotd.rs by sorting all 180 rotated peaks and taking the proper middle-pair average.
  • Add extensive Rust proptest coverage around RotD invariants (sqrt(2) bound, tightness for linearly polarised records, and serial vs parallel equivalence).
  • Minor Python refactors/cleanups (safer defaults for CLI + API parameters, metadata loop simplifications, typing suppression updates) and dependency bumps.

Reviewed changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src-rust/rotd.rs Fixes RotD median computation and adds property-based tests for invariants and parallel/serial agreement.
Cargo.toml Updates Rust deps (ndarray/numpy/pyo3) and adds proptest for new tests.
Cargo.lock Lockfile updates to reflect Rust dependency changes.
IM/im_calculation.py Avoids mutable default argument for ims_list and updates type-suppression comments.
IM/scripts/calculate_ims.py Avoids mutable default list options by defaulting periods/frequencies to None and filling them at runtime.
IM/im_reader.py Minor cleanup to use loop variables when setting coordinate/IM metadata descriptions.
IM/ims.py Updates type-suppression comment style and removes an unnecessary suppression on os.environ.update.
IM/snr_calculation.py Updates type-suppression comment style for DataFrame column assignment.
tests/test_ims.py Small test cleanups, updated type-suppression comment style, and minor dtype tightening in one hypothesis case.
tests/test_im_reader.py Minor iteration cleanup (for im_name in IM_METADATA vs .keys()).
pyproject.toml Bumps minimum supported Python version to 3.12.
.github/workflows/pr-review.yml Removes an “AI Code Review” reusable-workflow integration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pyproject.toml
@lispandfound
lispandfound merged commit 76c9d5e into master Jul 24, 2026
9 checks passed
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