Skip to content

fix: xNoiseSanPlot refined negative noise read from wrong array#386

Open
lpatiny wants to merge 4 commits into
mainfrom
fix-noise-san-plot
Open

fix: xNoiseSanPlot refined negative noise read from wrong array#386
lpatiny wants to merge 4 commits into
mainfrom
fix-noise-san-plot

Conversation

@lpatiny

@lpatiny lpatiny commented Jul 9, 2026

Copy link
Copy Markdown
Member

Bug

In the refine branch of xNoiseSanPlot, the refined negative noise level was read from cloneSignPositive (the positive signal) while indexing with cloneSignNegative.length — a copy-paste error from the positive branch just above. With refine: true (the default), the reported negative level did not reflect the negative signal at all.

The fix reads from cloneSignNegative and negates it, mirroring the initial negative-noise formula (-1 * signNegative[...]). Added a regression assertion: the refined negative level is now positive and of the same order as the positive one.

All 2230 tests pass.

For your review, @jobo322 — a second, separate issue I did not touch

determineCutOff accumulates averageValue as a plain sum and then uses it as a mean in (element[1] - averageValue)², so kiSqrt isn't the intended dispersion measure and the chosen cut-off is likely skewed. Dividing averageValue by elementsOfCutOff.length looks correct, but it changes the magnitude-mode SNR on the test spectrum from ~40.2 to ~63.5 — a real behavioral change to the published-method output. I left it as-is since that needs your domain judgment on what the reference value should be. Happy to open a follow-up if you confirm the intended behavior.

The refined negative noise level was indexed out of cloneSignPositive
(the positive signal) using the negative clone's length, a copy-paste
error. It now reads from cloneSignNegative and negates it, matching the
initial negative-noise formula, so with refine enabled the reported
negative level reflects the negative signal.
@lpatiny lpatiny requested a review from jobo322 July 9, 2026 05:38
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.15%. Comparing base (9fed281) to head (2c12305).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #386      +/-   ##
==========================================
+ Coverage   97.13%   97.15%   +0.02%     
==========================================
  Files         207      209       +2     
  Lines        4147     4148       +1     
  Branches     1036     1030       -6     
==========================================
+ Hits         4028     4030       +2     
+ Misses        115      114       -1     
  Partials        4        4              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread src/x/xNoiseSanPlot.ts Outdated
noiseLevelNegative =
cloneSignPositive[
-1 *
cloneSignNegative[

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems to me that cutOffSignalsIndexNeg would be always zero. because cutOffSignals defined positive. So there is not a refinning for noiseLevelNegative.

@jobo322

jobo322 commented Jul 10, 2026

Copy link
Copy Markdown
Member

Correct sign handling for negative noise in xNoiseSanPlot and uses cutoff for negative using negative data

Experiment Refactored Current state
DEPT dept-using-negative dept-current
1H 1H-using-negative 1H-current

@jobo322

jobo322 commented Jul 10, 2026

Copy link
Copy Markdown
Member

@lpatiny could you have a look, I propose use negative data to find the negative noise level

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.

2 participants