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

Harmonic oscillation reduction #285

Closed

Conversation

sarajamal57
Copy link

To solve the overflow problem in issue #284 :

new py file cesium/features/lomb_scargle_norm.py incorporating in lomb_scargle_model()

  • normalization of the input data if opt_normalize
  • computation of the Lomb-Scargle model via fit_lomb_scargle()
  • rescaling of the output dictionary via rescale_lomb_model()

Notes

  1. modifications are solely made in cesium/features/lomb_scargle_norm.py not in the original source file cesium/features/lomb_scargle.py
  2. successful local py tests for checking the rescaled output variables in the lomb_scargle_model.

Additional output
The full power spectrum and associated frequencies are added in the output dictionary in fit_lomb_scargle() [Lines 299-302]

@pep8speaks
Copy link

pep8speaks commented Apr 17, 2019

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

Line 56:81: E501 line too long (86 > 80 characters)
Line 59:5: E266 too many leading '#' for block comment
Line 164:5: E266 too many leading '#' for block comment
Line 173:81: E501 line too long (82 > 80 characters)
Line 384:5: E266 too many leading '#' for block comment
Line 386:81: E501 line too long (83 > 80 characters)
Line 390:5: E266 too many leading '#' for block comment
Line 401:81: E501 line too long (84 > 80 characters)
Line 402:81: E501 line too long (87 > 80 characters)
Line 407:81: E501 line too long (81 > 80 characters)
Line 438:9: W503 line break before binary operator
Line 439:9: W503 line break before binary operator
Line 476:9: W503 line break before binary operator
Line 485:81: E501 line too long (86 > 80 characters)
Line 494:81: E501 line too long (87 > 80 characters)

Comment last updated at 2021-05-27 22:37:18 UTC

@acrellin
Copy link
Member

@sarajamal57 Reviving this long-neglected PR (sorry!). I've moved your changes into the existing lomb_scargle.py file -- does the diff here look right to you? And thanks again for your contribution!

@acrellin acrellin requested review from stefanv and profjsb May 27, 2021 18:02
@@ -3,7 +3,7 @@
from ._lomb_scargle import lomb_scargle


def lomb_scargle_model(time, signal, error, sys_err=0.05, nharm=8, nfreq=3, tone_control=5.0):
def lomb_scargle_model(time, signal, error, sys_err=0.05, nharm=8, nfreq=3, tone_control=5.0, opt_normalize=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra parameter needs a docstring entry

def rescale_lomb_model(norm_model, mmean, mscale):
""" Rescale Lomb-Scargle model if compiled on normalized data.

Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

docstring needs formatting

@acrellin acrellin requested a review from stefanv May 27, 2021 20:30
@acrellin acrellin force-pushed the harmonic-oscillation-reduction branch from 0adacac to 536a05d Compare May 27, 2021 20:32
@acrellin acrellin force-pushed the harmonic-oscillation-reduction branch from e6645c4 to b8cc285 Compare May 27, 2021 22:31
nfreq : int
Number of frequencies to fit.
opt_normalize : boolean, optional
Defaults to False.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does opt_normalize do?

We also need tests for these new flags.

Copy link
Author

@sarajamal57 sarajamal57 May 31, 2021

Choose a reason for hiding this comment

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

The normalization resolves the numerical overflow observed for instance in issue #284

Modified/Added functions

  • lomb_scargle_model()
  • rescale_lomb_model()

Procedure

If opt_normalize==False, this refers to the original Lomb-Scargle periodogram function.

Otherwise:
Step 1 - the time-serie (TS) is normalized, and the (mean, std scale) are stored
Step 2 - Lomb-Scargle periodogram & the best fitted model are computed on the normalized (signal, error)
Step 3 - Results are scaled back to the original/raw format using the stored (mean, scale)

Step 1
In function lomb_scargle_model():

### Normalization ###
    mmean  = np.nanmean(signal)
    mscale = np.nanstd(signal)
    if opt_normalize:
        signal = (signal-mmean)/mscale
        error  = error/mscale

Step 2
The lomb-scargle (LS) periodogram is estimated on the normalized time-serie (signal, associated error).

Step 3
The best fitted Lomb-Scargle model is rescaled back.
In function lomb_scargle_model() :

current_fit = rescale_lomb_model(fit, mmean, mscale) if (opt_normalize&(i==0)) else fit 
## function rescale_lomb_model() is used to rescale the best fitted model (computed on normalized TS)

The first component (i==0) of var freq_fits requires to re-inject the scale

if (opt_normalize&(i==0)):
            model_dict['freq_fits'][-1]['resid']*=mscale

@profjsb
Copy link
Contributor

profjsb commented Jun 9, 2021

@sarajamal57 will you be able to add the tests/comments as per @stefanv's review?

@sarajamal57
Copy link
Author

yes, will do, no problem !
Commented text seems to be in place in the only modified file lomb_scargle_model() (cesium/features/lomb_scargle.py)
I will add a py test def test_lomb_scargle_normalize() to cesium/features/tests/test_lomb_scargle_features.py

@profjsb profjsb force-pushed the harmonic-oscillation-reduction branch from 2333327 to eb22bd6 Compare February 23, 2023 21:12
@profjsb profjsb closed this Feb 23, 2023
@profjsb
Copy link
Contributor

profjsb commented Feb 23, 2023

Closed in favor of #313

profjsb added a commit that referenced this pull request Mar 9, 2023
- Add the ability compute L-S models on normalized data.
- Adds periodogram to the outputs (not just the features computed on the periodogram)

Completed work by S. Jamal in PR #285
Co-authored-by: Sara Jamal <[email protected]>
Co-authored-by: Ari Crellin-Quick <[email protected]>
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.

5 participants