Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problem with single power spectrum #797

Merged
merged 12 commits into from
Feb 20, 2024
Merged

Conversation

matteobachetti
Copy link
Member

@matteobachetti matteobachetti commented Feb 7, 2024

Resolve #663

Note for reviewers: I created a test reproducing exactly the MWE by @dhuppenkothen in #663, and a similar one for cross spectra. My first solution broke other tests, so I changed slightly the logic in fourier.py and crossspectrum.py to make everything work.
In particular, if a segment size is almost an integer number of light curve bins (less than 1% away), instead of approximating to the lower bin I approximate to the higher one. Not more than that, because a bin with a significantly lower flux could alter the statistical properties of the data. New function in utils.py for this

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d47c95b) 96.37% compared to head (19d0c15) 96.27%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #797      +/-   ##
==========================================
- Coverage   96.37%   96.27%   -0.11%     
==========================================
  Files          44       44              
  Lines        8824     8855      +31     
==========================================
+ Hits         8504     8525      +21     
- Misses        320      330      +10     

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

Copy link
Collaborator

@mgullik mgullik left a comment

Choose a reason for hiding this comment

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

Hi @matteobachetti

I hope I understood what this PR is doing.
Isn't it better to check what is in the "last" bin that we are now considering if segment size is almost an integer number of light curve bins?
Since the segment size could be set as the GTI length, if the flux in the last bin is zero, it probably means that the GTI is set such as that bin shouldn't be included. Do you agree?

Moreover, I didn't understand what happended when there are multiple GTIs and segment size Is None. It seems that segment size becomes gti[-1, 1] - gti[0, 0] which would return an error (I hope) since the segment size is larger than all GTIs.

I would add tests with multiple GTIs in the case segment size is not set.

stingray/fourier.py Show resolved Hide resolved
Copy link
Member

@dhuppenkothen dhuppenkothen left a comment

Choose a reason for hiding this comment

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

LGTM overall. A couple of minor questions, but I don't think they should stop merging.

stingray/crossspectrum.py Show resolved Hide resolved
stingray/crossspectrum.py Outdated Show resolved Hide resolved
stingray/fourier.py Show resolved Hide resolved
@matteobachetti
Copy link
Member Author

@mgullik , @dhuppenkothen I think I've addressed all your comments.

Quick answers to @mgullik 's question:

Since the segment size could be set as the GTI length, if the flux in the last bin is zero, it probably means that the GTI is set such as that bin shouldn't be included. Do you agree?
Moreover, I didn't understand what happended when there are multiple GTIs and segment size Is None. It seems that segment size becomes gti[-1, 1] - gti[0, 0] which would return an error (I hope) since the segment size is larger than all GTIs.

This is a good point, and the code takes care of it. The GTIs always account for the bin size, so ideally they go from lc.time[0] - dt/2 to lc.time[-1] + dt / 2.

@matteobachetti matteobachetti added this pull request to the merge queue Feb 20, 2024
Merged via the queue into main with commit 5777271 Feb 20, 2024
14 of 17 checks passed
@matteobachetti matteobachetti deleted the fix_single_powerspectrum branch February 20, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in Powerspectrum when GTI is shorter than light curve?
3 participants