fix: trend detection for normalised time series#79
fix: trend detection for normalised time series#79ChrisMarsden833 wants to merge 6 commits intodevelopfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
RussellSB
changed the title
RussellSB
changed the title
There was a problem hiding this comment.
Looks great. Thanks so much for this. I left some nitpick comments which shouldnt take much work. What might take some time though is adding tests.
For this change we will need to add 2 tests.
Test 1
Test for the main focus of this commit, your [0,1] signal.
- Could make a new module to add tests to called something like
tests/tests_normalised_trends - You can reference your test data in a local csv somewhere under tests. E.g. save the csv
tests/tests_normalised_trends/normalised_data.csvand have a test file that calls ittests/test_normalised_trends.py - You can follow the similar pattern of most tests, e.g.
test_core_cases.py: line 18. The idea is to load the signal, detect trends, and then assess that the detected segments are as expected.
Test 2
Test for covering the lines of debug statements.
- A bit silly, but there's hard enforced rules to maintain the maximized code coverage to avoid missing any new edge cases.
- This test could work with any data really .... To make things simple could use same test data above and just make sure that once debug True is enabled that it runs plt show calls from within process_trends().
- You can refer to
test_plot_pytrendy_edgecases.py: line 114on how to do this with monkey patching. By setting detect trends plot=False, debug=True you should expect plt show to be hit from the debug True lines.
RussellSB
changed the title
|
Just realised there are some other nitpick changes to be resolved here, will resolve these too. |
…code coverage in abrupt_shaving.py.
|
I think I've addressed everything, see what you think. |
| from .detect_trends import detect_trends | ||
| from .io.data_loader import load_data | ||
| from .io.plot_pytrendy import plot_pytrendy | ||
| from .io.results_pytrendy import PyTrendyResults |
There was a problem hiding this comment.
Nitpick: is this change necessary?
There was a problem hiding this comment.
It's necessary for type hinting the test functions; for the same reason I figured exposing it was a good idea, so people can type hint their own functions.
Can remove if desired.
There was a problem hiding this comment.
Fair, but for that they could just do from pytrendy.io.results_pytrendy import PyTrendyResults and use that.
By design, I'd rather keep the accessible methods in the main __init__.py specifically for user-facing functions
| init_segment = init_segments[i] | ||
| is_not_prev_trend = 'trend_class' not in init_segment # edge case, in case not trend before | ||
| is_not_reclassified = is_not_prev_trend or segment['trend_class'] == init_segment['trend_class'] | ||
| if is_not_reclassified: |
There was a problem hiding this comment.
Nitpick: is this change necessary?
There was a problem hiding this comment.
No, but I was getting 99% code coverage because of this line, it was annoying me.
There was a problem hiding this comment.
Weird. The previous version with the linebreak achieves 100% code coverage in other recent PRs.
Maybe it was showing 99% locally cause not all tests were ran? Or a diff metric from codecov was referred to?
| """ | ||
| Tests for debug functionality. | ||
|
|
||
| These tests verify that the debug process works as expected. |
There was a problem hiding this comment.
I'm not sure these equivalency tests are the best fit here. The logic from the monkey patch isn't getting used.
In 'test_plot_pytrendy_edgecases.py: line 114' , the assert method checks that the plt.show() gets called once from plot pytrendy as expected. I think if you rework this to be one test on gradual with debug enabled, then assert that the show calls == 7 (I believe that's the number of plt.show() calls in debug). That should be enough.
That way it will be testing that the debug code is working, that it plots the number of debug figures as expected.
There was a problem hiding this comment.
I didn't realise this - I thought it was just to suppress plot.show(). I'll look into fixing this as you suggest.
2 participants
Potential fix for 77. Needs review.
Copilot summary:
This pull request introduces a debug mode for the trend detection pipeline and refines the method for detecting trends in time series data. The debug mode provides additional plots and print statements for developers, while the trend detection logic is improved by using a data-driven threshold based on the interquartile range (IQR) of the signal. The most important changes are grouped below:
Debug Mode Enhancements:
debugparameter to thedetect_trendsandprocess_signalsfunctions, allowing developers to enable detailed debugging output, including additional plots and print statements. [1] [2] [3] [4] [5]process_signalsthat visualizes intermediate signal processing steps whendebug=True, aiding in development and troubleshooting.Trend Detection Improvements:
THRESHOLD_SMOOTH) from a fixed value to a fraction of the signal's IQR, making the detection logic more adaptive to the data's variability. [1] [2]