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 compute rms #792

Merged
merged 23 commits into from Jan 29, 2024
Merged

Fix compute rms #792

merged 23 commits into from Jan 29, 2024

Conversation

matteobachetti
Copy link
Member

@matteobachetti matteobachetti commented Jan 26, 2024

  • Make the code for rms computation a little more clear
  • deprecate old behavior
  • Check documentation --> Small update to Spectral Timing Exploration notebook

I also had to fix a couple of things related to warnings, because the new pytest is much pickier. All the changes to warnings (and to imports in the top of files) are due to that. Please ignore for the sake of this PR. Only consider changes to (test_)crossspectrum, powerspectrum, fourier, and minor related changes in multitaper, lombscargle and deadtime.

Resolve #790

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6f38ba2) 78.98% compared to head (02fe583) 95.83%.

❗ Current head 02fe583 differs from pull request most recent head 10764e4. Consider uploading reports for the commit 10764e4 to get more accurate results

Files Patch % Lines
stingray/crossspectrum.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #792       +/-   ##
===========================================
+ Coverage   78.98%   95.83%   +16.84%     
===========================================
  Files          43       43               
  Lines        8762     8784       +22     
===========================================
+ Hits         6921     8418     +1497     
+ Misses       1841      366     -1475     

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

@pep8speaks
Copy link

pep8speaks commented Jan 27, 2024

Hello @matteobachetti! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1713:101: E501 line too long (101 > 100 characters)
Line 1734:101: E501 line too long (101 > 100 characters)

Comment last updated at 2024-01-29 14:44:59 UTC

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,

very nice! This will make the rms calculation much stronger, especially the error estimate.

I noticed that in the tests of get_rms_from... you build your synthetic PDS from a model shape and then randomising the values. Is this procedure different from simulating the light curve from the same theoretical shape of the PDS (with Timmer&Koenig method) and calculating the PDS from the light curve?
Does it make sense to perform all the tests using this last way? It seems closer to the usual procedure that calculates the rms.

stingray/fourier.py Show resolved Hide resolved
stingray/powerspectrum.py Outdated Show resolved Hide resolved
@matteobachetti matteobachetti added this pull request to the merge queue Jan 29, 2024
Merged via the queue into main with commit e465bdd Jan 29, 2024
14 checks passed
@matteobachetti matteobachetti deleted the fix_compute_rms branch January 29, 2024 18:08
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.

Errors in rms_calculation are incorrect
3 participants