-
Notifications
You must be signed in to change notification settings - Fork 18
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
fourier series: more consistent with standard definition #512
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #512 +/- ##
=======================================
Coverage 49.76% 49.76%
=======================================
Files 50 50
Lines 3563 3563
=======================================
Hits 1773 1773
Misses 1790 1790
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
To mention the motivation behind this - if we disregard the covariates we can use fft to estimate the coefficients with is much faster. With this change the coeffs are much easier to compare: import numpy as np
import mesmer
# create test data
n_years = 5
n_months = n_years * 12
months = np.tile(np.arange(12), n_years)
alpha = 2 * np.pi * months / 12
monthly_target = 3.14 * np.cos(alpha) + 2.72 * np.sin(alpha) + np.random.randn(n_months)
# use mesmer to estimate coeffs w/o covariates
yearly_predictor = np.zeros(n_months)
coeffs, mse = mesmer.stats._harmonic_model._fit_fourier_coeffs_np(
yearly_predictor,
monthly_target,
first_guess=np.zeros(4)
)
print(coeffs)
# use fft to estimate coeffs
freq = np.fft.rfftfreq(n_years * 12, 1/12)
sel = freq == 1.
coeffs_rfft = np.fft.rfft(monthly_target) / n_months
a_1 = 2 * coeffs_rfft[sel].real
b_1 = -2 * coeffs_rfft[sel].imag
# compare them
np.testing.assert_allclose(coeffs[1], a_1, rtol=1e-5)
np.testing.assert_allclose(coeffs[3], b_1, rtol=1e-5)
|
Okay, but can we disregard the covariates? |
I don't know1. It is a bit academic but helped me to understand the whole thing better. We could use fft to compute the For me this is a bit cleaner in any case & not very costly as it's not released in this form. Actually, I just wondered if we should reorganize coeffs even a bit more?
1We could find out if the covariates can be disregarded, also computing the BIC or deviance with and without the covariates. |
Ok let's keep the order of coeffs as suggested here (cosT, cos, sinT, sin). Is the PR ok for you, @veni-vidi-vici-dormivi? |
Yes definitely thank you, but maybe we should open an issue with all the information of this PR and #516? |
I thought if there was a better order of the coeffs for point 4 in #519 - but I did not come to any conclusion. So I suggest we merge as is and leave that for the future (if ever...). |
…p#512) * fourier series: more consistent with standard definition * changelog
CHANGELOG.rst
Make our fourier series more consistent with the standard usage (e.g. on wikipedia for the fourier series or in numpy for fft1), defined as:
So this PR does two things:
1 well fft uses the exponential form but that's equivalent