Fix power upper limit #946
Conversation
Changed the minimising method called in scipy.optimize.minimize to "Nelder-Mead" Additionally, updated the bounds to exclude 0 from the non-central chi2 distribution. Included a flag in the function to potentially solve issue StingraySoftware#942
from black stingray
|
@yashbhargava1992 Please discard all changes that are not strictly related to this PR, or it will break all other open PRs 😅 |
There was a problem hiding this comment.
Hi @yashbhargava1992 , good job, almost done! Please add a new test in test_stats.py to verify that this is working as expected. Also, please check that other functions calling this one are using the correct keyword (e.g. amplitude upper limit)
|
@yashbhargava1992 please also update your branch, I fixed one of the issues with testing |
Do I pull from the upstream main? |
Yes, rebase would be better but pull is ok too |
Modified modulation_upper_limit to utlise the summed flag option.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #946 +/- ##
===========================================
- Coverage 96.06% 79.96% -16.10%
===========================================
Files 48 48
Lines 9890 9895 +5
===========================================
- Hits 9501 7913 -1588
- Misses 389 1982 +1593 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stingray/stats.py
Outdated
| power in a QPO, the n in Z^2_n or the number of averaged PDS | ||
| c: float | ||
| The confidence value for the probability (e.g. 0.95 = 95%) | ||
| summed_flag: bool |
There was a problem hiding this comment.
I think we should be explicit in examples here, stating that, typically, Z^2_n will have summed_flag=True and powers will have summed_flag=False
| Ratio of the frequency of this feature with respect to the Nyquist | ||
| frequency. Important to know when dealing with FFTs, because the FFT | ||
| response decays between 0 and f_Nyq similarly to the response inside | ||
| a frequency bin: from 1 at 0 Hz to ~2/pi at f_Nyq |
There was a problem hiding this comment.
The new flag should also be documented here
stingray/tests/test_stats.py
Outdated
| assert np.isclose(power_upper_limit(40, 1, 0.99), 75, rtol=0.1) | ||
|
|
||
| def test_power_upper_limit_averaging(self): | ||
| assert np.isclose(power_upper_limit(100, 10, 0.997, summed_flag=False), 115, rtol=0.1) |
There was a problem hiding this comment.
There should be a new line after the function
stingray/tests/test_stats.py
Outdated
| def test_power_upper_limit(self): | ||
| # Use example from Vaughan+94 | ||
| assert np.isclose(power_upper_limit(40, 1, 0.99), 75, rtol=0.1) | ||
|
|
There was a problem hiding this comment.
Please add a test also for amplitude_upper_limit
Updated version to v2.2.10 and added new features and bug fixes.
991e488
2 participants
Relevant Issue(s)/PR(s)
Fixes #945 and #942
Provide an overview of the implemented solution or the fix and elaborate on the modifications.
Updated the method used in minimize function to Nelder-Mead to ensure convergence in test cases.
Added a flag to switch between summed and average power for determining upper limit in different cases.