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

Improving periodogram.flatten() for SNRPeriodogram purposes. #1307

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ojhall94
Copy link
Contributor

@ojhall94 ojhall94 commented May 1, 2023

This work in progress pull request is making a number of changes to the Periodogram module, in order to improve the functionality of the .flatten() function. List of changes made and to be made:

  • Add a diagnose_flattening() function to the SNRPeriodogram. Reasoning: there is little transparency on what the Periodogram.flatten() function does to the periodogram power, unless the user diagnoses this themselves. This makes that process more straightforward.
  • Make a more intelligent LombScarglePeriodogram.flatten() function that accounts for whether power excess is at low or high frequency, so that it requires less trial and error by users.
  • Update the unit tests to account for the new and changed features.
  • Copy Periodogram.flatten() to LombScarglePeriodogram. Reasoning: the BLSPeriodogram has its own flatten() function, and Periodogram.flatten() was only intended for use on LombScarglePeriodograms. Periodogram.flatten() should be deprecated.
  • Add a deprecation warning to the Periodogram.flatten() function.

Discussion points:

  • Should we keep Periodogram.flatten() in case users load in their own periodograms? Or should we encourage users to read those in as a LombScarglePeridogram?

@ojhall94
Copy link
Contributor Author

ojhall94 commented May 1, 2023

After discussing with @christinahedges we decided to keep the .flatten() code in the Periodogram module, as people may want to use it using their own data that isn't strictly generated using a LombScarglePeriodogram (such as TESS-SIP, which generates a Periodogram object but doesn't use Lomb Scargle).

@ojhall94
Copy link
Contributor Author

ojhall94 commented May 1, 2023

Here's an example of the new SNRPeriodogram.diagnose_flatten() function. Users can use this to check whether the .flatten() function is removing power from the modes of oscillation they're looking at and adjust the .flatten() parameters accordingly.

The .flatten() keyword argument filter_width has been increased to 0.1 for better performance - but this isn't intelligent and needs to be further improved.

image

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.

None yet

1 participant