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 powerspectrum from lightcurve - wrong size, float casting #760

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

matteobachetti
Copy link
Member

@matteobachetti matteobachetti commented Sep 27, 2023

Resolves two recently reported issues:

  • avg_*s_from_event methods not accepting complex fluxes and
  • avg_*s_from_event methods throwing errors of "no segment shorter than gti"

The first one comes from our forcing a float casting on input fluxes (to avoid integers - that can lead to casting errors in Numba routines, and quadruple precision, which is overkill), that kills complex numbers.

The second one comes from our re-defining dt to accommodate an exact number of bins inside a segment. This works well with actual events, but does damage when you have a light curve that was sampled with a given dt.

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #760 (9267e68) into main (5fac201) will increase coverage by 0.62%.
The diff coverage is 100.00%.

❗ Current head 9267e68 differs from pull request most recent head 962543a. Consider uploading reports for the commit 962543a to get more accurate results

@@            Coverage Diff             @@
##             main     #760      +/-   ##
==========================================
+ Coverage   96.62%   97.25%   +0.62%     
==========================================
  Files          41       41              
  Lines        7388     7396       +8     
==========================================
+ Hits         7139     7193      +54     
+ Misses        249      203      -46     
Files Coverage Δ
stingray/fourier.py 99.76% <100.00%> (+<0.01%) ⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -1846,11 +1850,13 @@ def avg_pds_from_events(
"""
if segment_size is None:
Copy link
Collaborator

@mgullik mgullik Sep 28, 2023

Choose a reason for hiding this comment

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

If we have this line, do you think it's better to set segment_size = None as the default value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it would indeed make sense. However, this would be a breaking change and would require to make a number of modifications to the rest of the code as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand, it makes sense

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,

great upgrade to the code!
If I understood correctly you define
dt = np.median(np.diff(times[:100]))
in the case dt is not given. I was wondering what happens if there are less than 100 bins in the time array? And also if the function checks whether or not the light curve is evenly sampled?

There are also a couple of comments attached to the file

@matteobachetti
Copy link
Member Author

times[:100] is safe if the array is shorter than 100, it will just give the full array. These functions assume that data are evenly sampled. Making the check every time would add significant computation in large datasets (checks of this kind were one of the reasons Stingray was so slow in the past 😅 )

@mgullik mgullik added this pull request to the merge queue Sep 28, 2023
Merged via the queue into main with commit 609482b Sep 28, 2023
5 checks passed
@matteobachetti matteobachetti deleted the fix_powerspectrum_from_lightcurve branch September 28, 2023 10:02
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