new feature peak-to-peak variation #332
Conversation
CodSpeed Performance ReportMerging #332 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Please rebase on master.
It is not clear what is the source of the desired values for random samples and sine. Could you please some comments to the code about this? For me it looks like this value should not converge with large n, could you explain why it is not true?
One more thing about the desired value. In the docs you say that threshold should be 0.1-0.15 and for non-variable data the value should be close to zero. Could you please justify these values in one or few tests, maybe modifying existing tests.
| n = 100 | ||
| t = np.arange(n) | ||
| m = np.ones_like(t) | ||
| sigma = 0.1 * np.ones_like(t) |
There was a problem hiding this comment.
| sigma = 0.1 * np.ones_like(t) | |
| sigma = np.full_like(t, 0.1) |
| sigma = 0.1 * np.ones_like(t) | ||
| actual = feature(t, m, sigma) | ||
| desired = 0.8 | ||
| assert_allclose(actual, desired, rtol=3 / np.sqrt(n)) |
There was a problem hiding this comment.
Why do we use rtol=3 / np.sqrt(n), I see no random number used...
| rng = np.random.default_rng(0) | ||
| n = 100 | ||
| t = np.linspace(0, 1, n) | ||
| m = np.abs(rng.normal(0, 1, n)) |
| n = 100 | ||
| t = np.linspace(0, 1, n) | ||
| m = np.abs(rng.normal(0, 1, n)) | ||
| sigma = 0.2 * np.ones_like(t) |
There was a problem hiding this comment.
| sigma = 0.2 * np.ones_like(t) | |
| sigma = np.full_like(t, 0.2) |
| sigma = 0.2 * np.ones_like(t) | ||
| feature = PeakToPeakVar() | ||
| actual = feature(t, m, sigma) | ||
| desired = 1 |
| assert_allclose(actual, desired, rtol=3 / np.sqrt(n)) | ||
|
|
||
|
|
||
| def test_ptpvar_norm_data_2(): |
| sigma = np.ones_like(t) | ||
| feature = PeakToPeakVar() | ||
| actual = feature(t, m, sigma) | ||
| desired = 1 |
| \frac{(m_i - \sigma_i)_\text{max} - (m_i + \sigma_i)_\text{min}}{(m_i - \sigma_i)_\text{max} + (m_i + \sigma_i)_\text{min}} | ||
| $$ | ||
| For non-variable data, it should be close to zero. | ||
| If data is close to be variable, the index should be more or equal than 0.10-0.15. |
There was a problem hiding this comment.
What does "close to be variable" mean? Could you please clarify text here? Maybe we should say something like "commonly used variability threshold lies in a range of 0.1-0.15"?
| $$ | ||
| For non-variable data, it should be close to zero. | ||
| If data is close to be variable, the index should be more or equal than 0.10-0.15. | ||
| It is sensitive to magnitude of error values and can be negative in overestimated errors case. |
There was a problem hiding this comment.
I don't think that we need this sentence here
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #332 +/- ##
=======================================
Coverage 85.04% 85.04%
=======================================
Files 9 9
Lines 2668 2668
=======================================
Hits 2269 2269
Misses 399 399 ☔ View full report in Codecov by Sentry. |
| from light_curve.light_curve_py import PeakToPeakVar | ||
|
|
||
|
|
||
| def test_ptpvar(): |
There was a problem hiding this comment.
I still don't really understand what this test tests. Should it work for any statistical feature, not this one only?
There was a problem hiding this comment.
We still need a test to make clear what "0.1-0.15" refers to
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Realisation of the new feature: peak-to-peak variation
issue: https://github.com/light-curve/light-curve-feature/issues/104