Skip to content

Conversation

@ydejiang
Copy link
Contributor

This PR adds per-subintegration normalization to the subints plot.
Each subint row is now scaled independently to the [0, 1] range.

This improves visibility when a single sub-int contains strong RFI.
Only the normalization step is changed; all plotting logic and layout remain unchanged.

@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.86%. Comparing base (50aaf4b) to head (b31acff).

Files with missing lines Patch % Lines
src/riptide/candidate.py 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #11      +/-   ##
==========================================
- Coverage   93.94%   93.86%   -0.09%     
==========================================
  Files          33       33              
  Lines        1800     1809       +9     
  Branches      139      141       +2     
==========================================
+ Hits         1691     1698       +7     
- Misses         70       71       +1     
- Partials       39       40       +1     

☔ View full report in Codecov by Sentry.
📢 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.

@v-morello
Copy link
Owner

v-morello commented Dec 2, 2025

Hi @ydejiang, thanks for the MR. I'm happy to merge this feature and perhaps make it a configurable option in the next release of riptide. A few high-level comments though:

  • You're doing peak-to-peak normalisation, but we could do whitening instead, i.e. for each subint subtract the mean and divide by stddev. I honestly don't have a strong optinion on which looks best in the plots, but maybe you want to try that.
  • Regardless of the normalisation scheme, it can be done without a for loop using numpy (see below).
  • No need to allocate a separate array
  • The atol trick is a hack and we can do better 😄

I suggest something along these lines (using whitening, but you can do the same with peak-to-peak normalisation)

means = X.mean(axis=1).reshape(-1, 1)
stddevs = X.std(axis=1).reshape(-1, 1) 
stddevs[stddevs == 0] = 1  # My replacement of the `atol` trick
# Note that this does not modify the original input array, this just shadows the X variable name with a new array
X = (X - means) / stddevs 

@ydejiang ydejiang closed this Dec 2, 2025
@ydejiang
Copy link
Contributor Author

ydejiang commented Dec 2, 2025

Hi Vincent, thank you so much for the detailed feedback!
Your example made the logic much clearer — your coding style is definitely more professional!

I tried both whitening and my original peak-to-peak normalisation on several examples.
For the diagnostic plots I tested, the simple peak-to-peak scaling actually produced slightly clearer contrast. So I’ve kept the peak-to-peak approach, but rewrote it following your suggestions — fully vectorised, no loop, and without the atol trick.

The code is much cleaner now.
Thanks again for your guidance, I really appreciate it!

@v-morello
Copy link
Owner

@ydejiang Thank you, I always like to talk about code with anyone. Can I ask why you closed the pull request though? I was expecting just one more commit with a better version of the logic and I would have happily merged that. In any case, looking forward to the next one.

@v-morello
Copy link
Owner

Oh, just saw you did open another PR already. All good 😄

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