feat: add weekly data support and rework date logic#205
feat: add weekly data support and rework date logic#205ChrisMarsden833 wants to merge 15 commits into
Conversation
…feature testing for non-datetime indexes, to make sure the results actually make sense
…or full code coverage.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR generalises trend detection and post-processing from date-only handling to internal index-based processing, with type-aware remapping back to external coordinates. Plotting, result summaries, and tests are updated to follow the new integer/float/string index paths. ChangesIndex-type generalisation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📚 Docs preview deployed! Your docs preview is available at: https://russellsb.github.io/pytrendy/pr-205/
|
RussellSB
changed the title
RussellSB
changed the title
RussellSB
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/conftest.py (1)
12-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring is now out of date for the new float/int index types.
The docstring still documents
start/endasstr or Timestamp, butassert_segments_matchnow also handles floats (and, pertest_non_dates.py, ints/strings). Worth a quick update so the doc matches behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 12 - 32, Update the docstring for assert_segments_match to match its current behavior: it now accepts numeric index types in addition to date-like values. Adjust the documented start/end types and examples in the helper’s docstring so it reflects the float/int/string handling used by the segment comparison logic and the related tests.pytrendy/post_processing/segments_refine/gradual_expand_contract.py (1)
82-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse integer arithmetic for the orphan check
segments_refined[i - 1]['end'],new_start, anddf.indexare integers at this stage.pd.to_datetime()turns those labels into timestamps, sodistanceis computed from nanosecond offsets andextremum in df.indexcan never succeed. The orphan-avoidance path is effectively inert. Keep this block on raw ints (extremum = new_start - 1,new_start -= 1).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pytrendy/post_processing/segments_refine/gradual_expand_contract.py` around lines 82 - 99, The orphan-check in the gradual expand/contract logic is using timestamp conversion on integer positions, which makes the distance math and index membership test ineffective. Update the block in gradual_expand_contract so it stays on raw integer labels for the segment boundary check: use the previous segment end, new_start, and df.index as ints, compute the extremum as one step before new_start, and decrement new_start by one when the orphan condition is met. Keep the existing direction and Noise handling in the same branch around the segment_refined[i - 1]['end'] / new_start logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytrendy/detect_trends.py`:
- Around line 101-102: The index remapping in detect_trends should be built from
positional values, not label-based Series access. Update the construction of
index_lookup in detect_trends.py so it uses the position sequence from
internal_index together with the positional values of external_index, rather
than external_index[i] lookups that depend on the original DataFrame index. This
change should be applied in the detect_trends logic where internal_index and
external_index are created, keeping the mapping correct for non-RangeIndex
inputs.
- Around line 98-105: The temporary index logic in detect_trends uses
date_col/None as a scratch column, which can overwrite a real None-named column
and affects the plot branch too. Update the index generation and restore flow in
detect_trends to use a dedicated private internal column name instead of
date_col when staging the generated index, then map it back only for the final
lookup/indexing steps. Keep the fix localized around the
internal_index/index_lookup setup and the set_index/reset handling so the
working copy never reuses the user’s date_col as temporary storage.
In `@pytrendy/io/plot_pytrendy.py`:
- Around line 42-44: Remove the leftover debug output from the index-type check
in the plotting function so it no longer writes to stdout on every call, and
simplify the membership test in that same block to use the cleaner “not in”
form. Update the logic in the plot-related code path that handles index type
validation so it keeps the same behavior without the debug print, using the
existing accepted_index_types check.
- Around line 83-88: The string branch in `plot_pytrendy` is checking adjacency
from the segment start instead of the segment end, which makes
`next_neighbouring` incorrect for multi-point segments. Update the
`next_neighbouring` logic in the `index_type == 'string'` path to compare the
next segment’s start against the current segment’s end, consistent with the date
and fallback branches, using the existing `next_seg`, `start`, `end`, and
`df.index` values in this block.
- Line 148: The string-index handling in the segment adjustment logic still uses
label arithmetic, which can crash when the index values are strings. Update the
branch in plot_pytrendy’s segment processing around the segments_enhanced
adjustment to use the positional index from df.index.get_loc(...) minus 1, then
translate that position back to the corresponding label before assigning to
segments_enhanced[i+1]['start'], matching the existing neighbour-check approach.
- Around line 119-131: The abrupt/noise neighbor adjustment in plot_pytrendy.py
still performs raw label arithmetic on string-indexed data, so fix the branch
that computes prev_new_end/new_end in the plotting logic around the segment
handling. Update the code in the segment loop and the “Adjust ends when
appropriate” block to use index positions via get_loc (or otherwise avoid adding
1 to string labels), and ensure the mask/range logic works for string indices
instead of assuming numeric/date arithmetic.
In `@pytrendy/io/results_pytrendy.py`:
- Around line 107-111: `print_summary`/`descriptor` currently falls back to
“dates” for float index types, so add an explicit `float` branch alongside the
existing `integer` and `string` handling in `results_pytrendy.py`; update the
descriptor selection logic in the same area where `self.index_type` is checked
so float indexes are reported with the correct label instead of “between dates.”
- Around line 77-83: The unit label logic in results formatting only
special-cases self.index_type == 'integer', so float and string index results
still get renamed to 'days'. Update the unit_descriptor selection in the
results-building code that renames the dataframe column and assembles cols so it
maps all supported index types from the whitelist (including float and string)
to the correct label instead of relying on a single if branch. Use the existing
self.index_type handling in the dataframe rename path to keep the header
consistent with the actual axis.
In `@pytrendy/post_processing/segments_refine/abrupt_shaving.py`:
- Around line 128-130: Remove the no-op self-assignments in abrupt_shaving.py
within the segment refinement flow: the meta_df construction already has the
desired values, so `meta_df['start'] = meta_df['start']` and `meta_df['end'] =
meta_df['end']` should be deleted. Check the surrounding
`segments_refined`/`meta_df` logic to confirm `start` and `end` are already
plain integers from the internal index and no datetime normalization is needed
before removing them.
In `@tests/conftest.py`:
- Around line 41-54: The `start` and `end` assertions in the comparison helper
are duplicated except for the field name, so extract a small shared helper in
`tests/conftest.py` to compare one segment value against the expected value with
the existing float-rounding behavior. Update the current repeated blocks to call
that helper for both `start` and `end`, keeping the existing assertion messages
and tolerance logic centralized for easier future changes.
In `@tests/test_non_dates.py`:
- Line 22: The test setup is using dict(...) in the method_params assignments,
which triggers the Ruff C408 lint rule. Update the relevant method_params
initializations in the non-dates test helper cases to use dictionary literals
instead of dict(...) while keeping the same keys and values, and apply the same
change to the other matching occurrences in this test file.
In `@tests/tests_plotting/edgecases/test_plot_pytrendy_edgecases.py`:
- Around line 72-79: The test setup in the edgecase plotting suite is
duplicating the same external_index/internal_index/index_lookup construction
used by detect_trends, which can drift if the production indexing logic changes.
Move this lookup-building pattern into a shared test helper (for example, in
conftest.py) and use that helper from the unwrapped-pipeline test so the test
stays aligned with detect_trends and any future changes only need to be updated
in one place.
---
Outside diff comments:
In `@pytrendy/post_processing/segments_refine/gradual_expand_contract.py`:
- Around line 82-99: The orphan-check in the gradual expand/contract logic is
using timestamp conversion on integer positions, which makes the distance math
and index membership test ineffective. Update the block in
gradual_expand_contract so it stays on raw integer labels for the segment
boundary check: use the previous segment end, new_start, and df.index as ints,
compute the extremum as one step before new_start, and decrement new_start by
one when the orphan condition is met. Keep the existing direction and Noise
handling in the same branch around the segment_refined[i - 1]['end'] / new_start
logic.
In `@tests/conftest.py`:
- Around line 12-32: Update the docstring for assert_segments_match to match its
current behavior: it now accepts numeric index types in addition to date-like
values. Adjust the documented start/end types and examples in the helper’s
docstring so it reflects the float/int/string handling used by the segment
comparison logic and the related tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8ee3dea6-f2b6-4277-bd09-3ee58871de3c
⛔ Files ignored due to path filters (1)
test.pngis excluded by!**/*.png
📒 Files selected for processing (18)
pytrendy/detect_trends.pypytrendy/io/plot_pytrendy.pypytrendy/io/results_pytrendy.pypytrendy/post_processing/segments_analyse.pypytrendy/post_processing/segments_get.pypytrendy/post_processing/segments_refine/__init__.pypytrendy/post_processing/segments_refine/abrupt_shaving.pypytrendy/post_processing/segments_refine/artifact_cleanup.pypytrendy/post_processing/segments_refine/gradual_expand_contract.pypytrendy/post_processing/segments_refine/segment_grouping.pypytrendy/post_processing/segments_refine/trend_classify.pypytrendy/post_processing/segments_refine/update_neighbours.pypytrendy/process_signals.pytests/conftest.pytests/test_core_cases.pytests/test_io_results.pytests/test_non_dates.pytests/tests_plotting/edgecases/test_plot_pytrendy_edgecases.py
📜 Review details
⚠️ CI failures not shown inline (1)
GitHub Actions: Tests / 0_Run Tests.txt: feat: flexibility for date granularity
Conclusion: failure
##[group]Run pytest tests/ -v -m core
�[36;1mpytest tests/ -v -m core�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib
##[endgroup]
============================= test session starts ==============================
platform linux -- Python 3.12.13, pytest-9.1.1, pluggy-1.6.0 -- /opt/hostedtoolcache/Python/3.12.13/x64/bin/python
cachedir: .pytest_cache
Matplotlib: 3.10.8
Freetype: 2.6.1
rootdir: /home/runner/work/pytrendy/pytrendy
configfile: pyproject.toml
plugins: timeout-2.4.0, cov-7.1.0, mpl-0.19.0
timeout: 15.0s
timeout method: signal
timeout func_only: False
collecting ... collected 103 items / 27 deselected / 76 selected
tests/test_core_cases.py::TestCoreCases::test_gradual_trends PASSED [ 1%]
tests/test_core_cases.py::TestCoreCases::test_abrupt_trends_no_padding PASSED [ 2%]
tests/test_core_cases.py::TestCoreCases::test_abrupt_trends_with_padding PASSED [ 3%]
tests/test_data_loader.py::TestDataLoader::test_load_series_synthetic_returns_dataframe PASSED [ 5%]
tests/test_data_loader.py::TestDataLoader::test_load_series_synthetic_has_data PASSED [ 6%]
tests/test_data_loader.py::TestDataLoader::test_load_series_synthetic_has_expected_columns PASSED [ 7%]
tests/test_data_loader.py::TestDataLoader::test_load_classes_signals_returns_dataframe PASSED [ 9%]
tests/test_data_loader.py::TestDataLoader::test_load_classes_signals_has_data PASSED [ 10%]
tests/test_data_loader.py::TestDataLoader::test_load_classes_signals_has_expected_columns PASSED [ 11%]
tests/test_data_loader.py::TestDataLoader::test_load_default_dataset PASSED [ 13%]
tests/test_data_loader.py::TestDataLoa...
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*: Before pushing, run the core test suite withpytest tests/ -m coreand then run the remaining tests withpytest tests/ -m "not core" --cov-append; CI also uses a 15-second per-test timeout.
Plot regression tests usepytest-mpl, and baselines must be regenerated only againstmatplotlib==3.10.8to avoid false failures.
Files:
tests/conftest.pytests/test_io_results.pytests/test_non_dates.pytests/test_core_cases.pytests/tests_plotting/edgecases/test_plot_pytrendy_edgecases.py
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdCompact context for OpenCode sessions. Deeper topics live in
.opencode/skills/(auto-loaded by trigger).What this is
PyTrendy: trend detection lib for time series. One public entrypoint:
pytrendy.detect_trends(df, date_col, value_col, ...)→PyTrendyResults. Python ≥3.10, CI on 3.12.Branch model (enforced by CI)
- PRs target
develop, notmain.check-base-branch.ymlhard-fails otherwise. Onlydevelop→mainrelease PRs and automateddocs/whats-new-*PRs bypass.main= stable release branch.develop= prerelease (devchannel).- Branch off:
git checkout -b my-feature origin/develop.Commits & PR titles (Conventional Commits, enforced)
lint-pr-title.ymlrejects titles not matching^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?!?: .+.- semantic-release maps:
feat→minor,fix→patch,!/BREAKING CHANGE:→major, others→no bump.- Deprecating a public API param =
feat:(minor), notrefactor:. Seemaintenanceskill.- Imperative, lowercase, <72 chars, no trailing period.
Install & verify
pip install -e ".[dev]" # dev: pytest, pytest-cov, pytest-mpl, pytest-timeout pip install -e ".[dev,docs]" # add mkdocs material + mkdocstrings for docs workVerify before push — CI runs core first, then non-core with
--cov-append:pytest tests/ -m core # essential, must pass pytest tests/ -m "not core" --cov-append # the rest # or just: pytest (pyproject addopts already set --mpl --cov=pytrendy)
- 15s per-test timeout (
pytest-timeout).- Plot tests use pytest-mpl with baselines pinned to matplotlib==3.10.8 (hard pin in
pyproject.toml). Regenerating baselines on another version = false failures. Seetestskill.coverage.xml+.coverageare gitignored artifacts; don't commit.Don't touch these manually
pyproject.tomlversion — bumped by semantic-release viapoetry version. E...
Files:
tests/conftest.pytests/test_io_results.pytests/test_non_dates.pypytrendy/post_processing/segments_refine/trend_classify.pypytrendy/post_processing/segments_get.pypytrendy/post_processing/segments_refine/segment_grouping.pypytrendy/process_signals.pypytrendy/post_processing/segments_refine/__init__.pypytrendy/post_processing/segments_refine/update_neighbours.pypytrendy/detect_trends.pypytrendy/io/plot_pytrendy.pypytrendy/post_processing/segments_refine/abrupt_shaving.pypytrendy/post_processing/segments_refine/gradual_expand_contract.pypytrendy/post_processing/segments_refine/artifact_cleanup.pytests/test_core_cases.pytests/tests_plotting/edgecases/test_plot_pytrendy_edgecases.pypytrendy/post_processing/segments_analyse.pypytrendy/io/results_pytrendy.py
pytrendy/**/*
📄 CodeRabbit inference engine (AGENTS.md)
The package implements a single public entrypoint,
pytrendy.detect_trends(df, date_col, value_col, ...), returningPyTrendyResults; changes underpytrendy/should preserve that public API surface.
Files:
pytrendy/post_processing/segments_refine/trend_classify.pypytrendy/post_processing/segments_get.pypytrendy/post_processing/segments_refine/segment_grouping.pypytrendy/process_signals.pypytrendy/post_processing/segments_refine/__init__.pypytrendy/post_processing/segments_refine/update_neighbours.pypytrendy/detect_trends.pypytrendy/io/plot_pytrendy.pypytrendy/post_processing/segments_refine/abrupt_shaving.pypytrendy/post_processing/segments_refine/gradual_expand_contract.pypytrendy/post_processing/segments_refine/artifact_cleanup.pypytrendy/post_processing/segments_analyse.pypytrendy/io/results_pytrendy.py
🪛 Ruff (0.15.20)
tests/test_non_dates.py
[warning] 22-22: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
[warning] 50-50: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
[warning] 78-78: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
pytrendy/process_signals.py
[warning] 92-92: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
pytrendy/post_processing/segments_refine/update_neighbours.py
[error] 24-24: Multiple statements on one line (colon)
(E701)
[error] 68-68: Multiple statements on one line (colon)
(E701)
pytrendy/detect_trends.py
[warning] 16-16: Too many branches (14 > 12)
(PLR0912)
[warning] 19-19: Boolean-typed positional argument in function definition
(FBT001)
[warning] 19-19: Boolean default positional argument in function definition
(FBT002)
[warning] 21-21: Boolean-typed positional argument in function definition
(FBT001)
[warning] 21-21: Boolean default positional argument in function definition
(FBT002)
pytrendy/io/plot_pytrendy.py
[warning] 9-9: Too many branches (46 > 12)
(PLR0912)
[warning] 9-9: Too many statements (135 > 50)
(PLR0915)
[warning] 9-9: Boolean-typed positional argument in function definition
(FBT001)
[warning] 9-9: Boolean default positional argument in function definition
(FBT002)
[error] 43-43: Test for membership should be not in
Convert to not in
(E713)
[warning] 95-95: Self-assignment of variable start
(PLW0127)
[warning] 128-131: Use ternary operator new_end = end + pd.Timedelta(days=1) if index_type == 'date' else end + 1 instead of if-else-block
Replace if-else-block with new_end = end + pd.Timedelta(days=1) if index_type == 'date' else end + 1
(SIM108)
[warning] 150-150: Self-assignment of variable end
(PLW0127)
[error] 171-171: Test for membership should be not in
Convert to not in
(E713)
[warning] 207-207: Boolean positional value in function call
(FBT003)
pytrendy/post_processing/segments_refine/abrupt_shaving.py
[warning] 84-84: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
pytrendy/post_processing/segments_refine/artifact_cleanup.py
[warning] 205-205: List index lookup in enumerate() loop
Use the loop variable directly
(PLR1736)
[warning] 209-209: List index lookup in enumerate() loop
Use the loop variable directly
(PLR1736)
[warning] 209-209: List index lookup in enumerate() loop
Use the loop variable directly
(PLR1736)
[warning] 219-219: List index lookup in enumerate() loop
Use the loop variable directly
(PLR1736)
[warning] 223-223: List index lookup in enumerate() loop
Use the loop variable directly
(PLR1736)
[warning] 227-227: List index lookup in enumerate() loop
Use the loop variable directly
(PLR1736)
[warning] 227-227: List index lookup in enumerate() loop
Use the loop variable directly
(PLR1736)
[warning] 339-339: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
[warning] 368-372: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
tests/tests_plotting/edgecases/test_plot_pytrendy_edgecases.py
[warning] 79-79: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
pytrendy/io/results_pytrendy.py
[warning] 116-116: Unnecessary else after return statement
Remove unnecessary else
(RET505)
🔇 Additional comments (14)
tests/test_core_cases.py (1)
11-11: LGTM!Also applies to: 41-41
tests/test_io_results.py (1)
404-404: LGTM!Also applies to: 419-426
tests/test_non_dates.py (1)
22-22: 🩺 Stability & AvailabilityNo CI failure from this deprecation
is_abrupt_paddedonly raises aDeprecationWarning, and pytest here is not configured to turn warnings into errors. The deprecation path is already covered by a dedicatedpytest.warns(DeprecationWarning)test.> Likely an incorrect or invalid review comment.pytrendy/process_signals.py (1)
58-58: LGTM!Also applies to: 91-99, 109-109, 128-135, 153-174
pytrendy/post_processing/segments_get.py (1)
65-72: LGTM!pytrendy/post_processing/segments_analyse.py (1)
56-56: LGTM!Also applies to: 70-70
pytrendy/post_processing/segments_refine/segment_grouping.py (1)
53-53: LGTM!Also applies to: 62-62
pytrendy/detect_trends.py (1)
132-142: 🎯 Functional CorrectnessNo fix needed for string date boundaries. The parsed-string
date_colpath intentionally keeps the original labels inresults.segments; the latepd.to_datetime(external_index)only prepares the plotting index. The core tests also expect'YYYY-MM-DD'segment boundaries on this path.> Likely an incorrect or invalid review comment.pytrendy/post_processing/segments_refine/update_neighbours.py (1)
11-96: LGTM!pytrendy/post_processing/segments_refine/__init__.py (1)
46-47: LGTM!pytrendy/post_processing/segments_refine/abrupt_shaving.py (1)
47-152: LGTM!pytrendy/post_processing/segments_refine/artifact_cleanup.py (1)
35-100: LGTM!Also applies to: 121-152, 193-228, 239-256, 330-385
pytrendy/post_processing/segments_refine/gradual_expand_contract.py (1)
29-72: LGTM!Also applies to: 102-115
pytrendy/post_processing/segments_refine/trend_classify.py (1)
45-46: LGTM!Also applies to: 79-79
| else: | ||
| external_index = np.arange(len(df)) | ||
|
|
||
| internal_index = np.arange(len(df)) | ||
| index_lookup = {internal_index[i] : external_index[i] for i in range(len(internal_index))} | ||
|
|
||
| df[date_col] = internal_index.copy() | ||
| df.set_index(date_col, inplace=True) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import pandas as pd, numpy as np
df = pd.DataFrame({'v':[1,2,3]})
df[None] = np.arange(3)
df.set_index(None, inplace=True)
print(df, "\nindex name:", df.index.name)
PYRepository: RussellSB/pytrendy
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -e
# Map the file and inspect the relevant ranges.
wc -l pytrendy/detect_trends.py
ast-grep outline pytrendy/detect_trends.py --view expanded
sed -n '1,220p' pytrendy/detect_trends.py | cat -n | sed -n '70,180p'Repository: RussellSB/pytrendy
Length of output: 3731
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,80p' pytrendy/detect_trends.py | cat -nRepository: RussellSB/pytrendy
Length of output: 4969
Use a dedicated scratch column for the generated index
date_col=None reuses None as temporary column storage. That clobbers any real None-named column in the working copy, and the plot branch does the same. Switch to a private internal name for the temporary index, then map it back only where needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pytrendy/detect_trends.py` around lines 98 - 105, The temporary index logic
in detect_trends uses date_col/None as a scratch column, which can overwrite a
real None-named column and affects the plot branch too. Update the index
generation and restore flow in detect_trends to use a dedicated private internal
column name instead of date_col when staging the generated index, then map it
back only for the final lookup/indexing steps. Keep the fix localized around the
internal_index/index_lookup setup and the set_index/reset handling so the
working copy never reuses the user’s date_col as temporary storage.
| internal_index = np.arange(len(df)) | ||
| index_lookup = {internal_index[i] : external_index[i] for i in range(len(internal_index))} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Series[i] is label-based (breaks on non-RangeIndex) vs positional zip.
python - <<'PY'
import pandas as pd, numpy as np
df = pd.DataFrame({'d':['a','b','c'],'v':[1,2,3]}, index=[10,11,12])
ext = df['d'].copy()
try:
print("Series[i]:", [ext[i] for i in range(len(df))])
except Exception as e:
print("Series[i] failed:", type(e).__name__, e)
print("zip:", list(zip(np.arange(len(df)), np.asarray(ext))))
PYRepository: RussellSB/pytrendy
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and nearby uses of index_lookup/external_index.
python3 - <<'PY'
from pathlib import Path
path = Path("pytrendy/detect_trends.py")
lines = path.read_text().splitlines()
for start, end in [(80, 125), (125, 170)]:
print(f"\n--- {path}:{start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PY
# Show where index_lookup is used elsewhere in the file.
rg -n "index_lookup|external_index|internal_index" pytrendy/detect_trends.pyRepository: RussellSB/pytrendy
Length of output: 3744
Build index_lookup from positions, not Series labels. external_index preserves df’s original index, so external_index[i] is label-based and breaks on any non-RangeIndex input. That makes the remap wrong or raises KeyError for valid frames.
🐛 Positional-safe lookup
- internal_index = np.arange(len(df))
- index_lookup = {internal_index[i] : external_index[i] for i in range(len(internal_index))}
+ internal_index = np.arange(len(df))
+ index_lookup = dict(zip(internal_index, np.asarray(external_index)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal_index = np.arange(len(df)) | |
| index_lookup = {internal_index[i] : external_index[i] for i in range(len(internal_index))} | |
| internal_index = np.arange(len(df)) | |
| index_lookup = dict(zip(internal_index, np.asarray(external_index))) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pytrendy/detect_trends.py` around lines 101 - 102, The index remapping in
detect_trends should be built from positional values, not label-based Series
access. Update the construction of index_lookup in detect_trends.py so it uses
the position sequence from internal_index together with the positional values of
external_index, rather than external_index[i] lookups that depend on the
original DataFrame index. This change should be applied in the detect_trends
logic where internal_index and external_index are created, keeping the mapping
correct for non-RangeIndex inputs.
|
|
||
| if isinstance(detected['start'], float): | ||
| assert round(detected['start'], 6) == round(expected['start'], 6), \ | ||
| f"Segment {i}: Expected start '{expected['start']}', got '{detected['start']}'" | ||
| else: | ||
| assert detected['start'] == expected['start'], \ | ||
| f"Segment {i}: Expected start '{expected['start']}', got '{detected['start']}'" | ||
|
|
||
| if isinstance(detected['end'], float): | ||
| assert round(detected['end'], 6) == round(expected['end'], 6), \ | ||
| f"Segment {i}: Expected end '{expected['end']}', got '{detected['end']}'" | ||
| else: | ||
| assert detected['end'] == expected['end'], \ | ||
| f"Segment {i}: Expected end '{expected['end']}', got '{detected['end']}'" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated comparison logic — extract a small helper.
The start/end blocks are identical apart from the key name. A tiny helper would cut the repetition and make future tweaks (e.g. changing tolerance) a one-line change.
♻️ Proposed refactor
- if isinstance(detected['start'], float):
- assert round(detected['start'], 6) == round(expected['start'], 6), \
- f"Segment {i}: Expected start '{expected['start']}', got '{detected['start']}'"
- else:
- assert detected['start'] == expected['start'], \
- f"Segment {i}: Expected start '{expected['start']}', got '{detected['start']}'"
-
- if isinstance(detected['end'], float):
- assert round(detected['end'], 6) == round(expected['end'], 6), \
- f"Segment {i}: Expected end '{expected['end']}', got '{detected['end']}'"
- else:
- assert detected['end'] == expected['end'], \
- f"Segment {i}: Expected end '{expected['end']}', got '{detected['end']}'"
+ def _check(key):
+ d, e = detected[key], expected[key]
+ if isinstance(d, float):
+ assert round(d, 6) == round(e, 6), \
+ f"Segment {i}: Expected {key} '{e}', got '{d}'"
+ else:
+ assert d == e, f"Segment {i}: Expected {key} '{e}', got '{d}'"
+
+ _check('start')
+ _check('end')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(detected['start'], float): | |
| assert round(detected['start'], 6) == round(expected['start'], 6), \ | |
| f"Segment {i}: Expected start '{expected['start']}', got '{detected['start']}'" | |
| else: | |
| assert detected['start'] == expected['start'], \ | |
| f"Segment {i}: Expected start '{expected['start']}', got '{detected['start']}'" | |
| if isinstance(detected['end'], float): | |
| assert round(detected['end'], 6) == round(expected['end'], 6), \ | |
| f"Segment {i}: Expected end '{expected['end']}', got '{detected['end']}'" | |
| else: | |
| assert detected['end'] == expected['end'], \ | |
| f"Segment {i}: Expected end '{expected['end']}', got '{detected['end']}'" | |
| def _check(key): | |
| d, e = detected[key], expected[key] | |
| if isinstance(d, float): | |
| assert round(d, 6) == round(e, 6), \ | |
| f"Segment {i}: Expected {key} '{e}', got '{d}'" | |
| else: | |
| assert d == e, f"Segment {i}: Expected {key} '{e}', got '{d}'" | |
| _check('start') | |
| _check('end') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` around lines 41 - 54, The `start` and `end` assertions in
the comparison helper are duplicated except for the field name, so extract a
small shared helper in `tests/conftest.py` to compare one segment value against
the expected value with the existing float-rounding behavior. Update the current
repeated blocks to call that helper for both `start` and `end`, keeping the
existing assertion messages and tolerance logic centralized for easier future
changes.
| external_index = pd.to_datetime(df[date_col]) | ||
| internal_index = np.arange(len(df)) | ||
| index_lookup = {internal_index[i] : external_index[i] for i in range(len(internal_index))} | ||
|
|
||
| df[date_col] = internal_index | ||
| df.set_index(date_col, inplace=True) | ||
| df = df[[value_col]] | ||
| method_params = {'abrupt_padding':0, 'avoid_noise':True} | ||
| method_params = dict(is_abrupt_padded=False, abrupt_padding=28, avoid_noise=True) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Test duplicates detect_trends's internal index/lookup construction.
This block re-implements the exact external_index / internal_index / index_lookup pattern from detect_trends.py to bypass grouping. That's a reasonable way to test the ungrouped pipeline, but it means any change to the internal indexing mechanism in detect_trends.py needs to be mirrored here manually, or this test will silently diverge and become misleading.
Consider extracting the lookup-construction snippet into a shared test helper (e.g. in conftest.py) so both this file and any future "unwrapped pipeline" tests stay in sync with a single source of truth.
Also applies to: 97-103
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 79-79: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tests_plotting/edgecases/test_plot_pytrendy_edgecases.py` around lines
72 - 79, The test setup in the edgecase plotting suite is duplicating the same
external_index/internal_index/index_lookup construction used by detect_trends,
which can drift if the production indexing logic changes. Move this
lookup-building pattern into a shared test helper (for example, in conftest.py)
and use that helper from the unwrapped-pipeline test so the test stays aligned
with detect_trends and any future changes only need to be updated in one place.
|
/oc some git diffs aren't entirely necessary in this PR - e.g. small changes targetting diff format changes here and there. Can you make a summary pointing them out? Also if a genuinely great addition to the package e.g. diff lib idea, but a bit of scope creep suggest making a new issue, for that to be addressed with more focus elsewhere instead. Any minor git diffs that can't be well argued for, suggest to clean up in the summary. |
PR Review Summary:
|
| File | Issue |
|---|---|
test.png |
Debug artifact — should be removed, not committed |
plot_pytrendy.py:43 |
Debug print statement left in (print(f"internal index type {index_type}")) — remove |
abrupt_shaving.py:128-130 |
No-op self-assignments (meta_df['start'] = meta_df['start']) — dead code from datetime purge |
test_non_dates.py:22,50,78 |
dict(...) calls trigger Ruff C408 — use literals {...} |
process_signals.py:92 |
Same dict() → literal issue |
update_neighbours.py:24,68 |
Ruff E701 — multiple statements on one line (use semicolons or split) |
Suggest Splitting Into Separate Issue/PR
The plot_pytrendy.py string-index branch has several correctness gaps:
- Line 86: adjacency check uses
startinstead ofend(will misfire on multi-point segments) - Lines 119-131: label arithmetic (
+ 1) on strings — willTypeError - Line 148:
segments_enhanced[i+1]['start'] - 1crashes on string indices
These are real bugs in the string-index plotting path. Rather than blocking the whole PR on fixing string plotting end-to-end, I'd suggest:
- Merge the core index generalisation (integer/float work correctly)
- Open a new issue: "String index plotting support is incomplete" — address the
get_loc-based arithmetic properly with dedicated tests
Lint Cleanup (Can Be Done In-Place)
results_pytrendy.py:83:unit_descriptoronly handlesinteger—float/stringget mislabelled as'days'results_pytrendy.py:111:descriptorfalls back to'dates'for float — should map to'indexes'conftest.py:41-54: Duplicated start/end comparison logic — extract a_check(key)helper
Recommended Action
- Remove
test.png, debug print, and no-op self-assignments - Fix the
dict()→ literal lint warnings (trivial) - Fix
results_pytrendy.pyto handle float/string descriptors (quick, correctness issue) - Open a follow-up issue for string-index plotting gaps
- Merge the rest — it's solid work
|
/oc tests are failing. Can you investigate why remaining tests are failing? Propose fixes - tested locally in your environment as reasonable and proven to resolve. Read the skills before trying and don't forget to address with the mindset that the skills repo offers. |
Co-authored-by: RussellSB <RussellSB@users.noreply.github.com>
|
Here's a summary of all fixes applied: Root Cause: 3 Failing TestsThe Root cause: Fix: Changed the orphan check to use plain integer arithmetic:
Additional CodeRabbit Fixes
Result: All 103 tests pass (76 core + 27 non-core). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/oc try address Chris' comment again:
I suggest you review previous attempts at this - where no PR was made or branch but you did provide summaries in comments of the changes you made but didnt save. Ensure you provide similar view but this time with links to the PRs you will actually make. |
SummaryI've successfully created 6 separate PRs targeting the
All 104 tests pass on each branch (77 core + 27 non-core). The PRs are structured so related changes go together for manageable review. Merge in order 1→6. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 participants
The aim of this addition is to create a system where the trend detection system isn't dependent on a datetime index, and instead relies on an integer index under the hood. This allows for any index to be used, be it different periods of time, or float or even string indexes/labels for unusual applications of pytrendy.
This PR needs some additional work to get the ball rolling, and doesn't quite have 100% test coverage (still) but the bulk of it is now there.