Add TotalLocatablePeriods query kind #7237
Conversation
WalkthroughIntroduces a new subscriber query class TotalLocatablePeriods that computes per-subscriber counts of locatable periods within a time window, supports spatial filtering, and allows deterministic exclusion of specific periods. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client
participant TLP as TotalLocatablePeriods
participant StartStops as _get_start_stops()
participant PeriodFilter as Period Exclusion
participant PerPeriodQuery as Per-period locatable query
participant SpatialUnit as Spatial Unit Filter
participant UnionAgg as Union / Aggregator
Client->>TLP: instantiate(start, total_periods, spatial_unit, periods_to_exclude...)
TLP->>StartStops: compute period start/stop pairs
StartStops->>PeriodFilter: normalize & filter excluded periods
PeriodFilter-->>TLP: deterministic period list
loop for each period
TLP->>PerPeriodQuery: query locatable subscribers for period
PerPeriodQuery->>SpatialUnit: apply spatial_unit filter
SpatialUnit-->>PerPeriodQuery: unique subscribers for period
end
PerPeriodQuery->>UnionAgg: union unique-per-period lists
UnionAgg-->>TLP: aggregated per-subscriber locatable period counts
TLP-->>Client: return per-subscriber counts
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
jc-harrison
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@flowmachine/flowmachine/features/subscriber/total_locatable_periods.py`:
- Around line 41-43: The docstring for the parameters period_length and
period_unit is misleading: period_length is described as "Total number of days
per period" even though period_unit accepts 'days', 'hours', or 'minutes';
update the docstring for period_length to be unit-agnostic (e.g., "Total number
of units per period" or "Length of each period in units specified by
period_unit") and ensure period_unit description remains consistent (e.g., "Unit
for period_length: 'days', 'hours', or 'minutes'"). Update these descriptions in
the function/class where period_length and period_unit are defined (search for
period_length and period_unit in total_locatable_periods.py) so the docstring
clearly indicates the relationship between the two parameters.
- Around line 77-94: The exclusion filtering silently ignores date-only strings
for finer-grained periods because standardise_date (via parse_datestring)
defaults them to 00:00:00; update the filtering in the periods_to_exclude block
to treat date-only inputs as whole-day ranges for hour/minute period_unit: when
iterating periods_to_exclude, detect inputs that lack time (use parse_datestring
or original string heuristic), and if period_unit is 'hour' or 'minute' expand
that date into a range (start_of_day..end_of_day) and include any period start
that falls within that range (instead of only matching 00:00:00); keep using
standardise_date for datetime inputs, preserve the existing
lower_bound/upper_bound logic (time_period_add, total_periods, period_length),
and ensure final self.periods_to_exclude remains sorted and deduplicated as
before.
🧹 Nitpick comments (1)
flowmachine/flowmachine/features/subscriber/total_locatable_periods.py (1)
122-137: Avoid double filtering of start/stop pairs.Lines 122–137 build
filtered_startstopsand then recompute the same filter to createfiltered_starts/filtered_stops. Reusing the first list keeps the logic in one place and avoids duplicate iteration.♻️ Refactor to reuse `filtered_startstops`
filtered_startstops = [ (start, stop) for start, stop in zip(starts, stops) if start not in self.periods_to_exclude ] if not filtered_startstops: raise ValueError( "Cannot create a TotalLocatablePeriods query with all periods excluded" ) - filtered_starts, filtered_stops = zip( - *( - (start, stop) - for start, stop in zip(starts, stops) - if start not in self.periods_to_exclude - ) - ) + filtered_starts, filtered_stops = zip(*filtered_startstops) return filtered_starts, filtered_stops
| period_length : int, default 1 | ||
| Total number of days per period. | ||
| period_unit : {'days', 'hours', 'minutes'} default 'days' |
There was a problem hiding this comment.
Clarify period_length docs for non‑day units.
Line 41–42 says “days per period”, but period_unit supports hours/minutes, so the current wording can mislead users. Please update the description to be unit‑agnostic.
✏️ Suggested docstring tweak
- period_length : int, default 1
- Total number of days per period.
+ period_length : int, default 1
+ Number of `period_unit` intervals per period.📝 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.
| period_length : int, default 1 | |
| Total number of days per period. | |
| period_unit : {'days', 'hours', 'minutes'} default 'days' | |
| period_length : int, default 1 | |
| Number of `period_unit` intervals per period. | |
| period_unit : {'days', 'hours', 'minutes'} default 'days' |
🤖 Prompt for AI Agents
In `@flowmachine/flowmachine/features/subscriber/total_locatable_periods.py`
around lines 41 - 43, The docstring for the parameters period_length and
period_unit is misleading: period_length is described as "Total number of days
per period" even though period_unit accepts 'days', 'hours', or 'minutes';
update the docstring for period_length to be unit-agnostic (e.g., "Total number
of units per period" or "Length of each period in units specified by
period_unit") and ensure period_unit description remains consistent (e.g., "Unit
for period_length: 'days', 'hours', or 'minutes'"). Update these descriptions in
the function/class where period_length and period_unit are defined (search for
period_length and period_unit in total_locatable_periods.py) so the docstring
clearly indicates the relationship between the two parameters.
| if periods_to_exclude is not None: | ||
| # Filter periods to exclude to only relevant dates | ||
| # (otherwise changes to irrelevant dates will change query ID) | ||
| lower_bound = standardise_date(start) | ||
| upper_bound = time_period_add( | ||
| start, total_periods * period_length, period_unit | ||
| ) | ||
| # Needs to be sorted so that the query ID is deterministic | ||
| self.periods_to_exclude = sorted( | ||
| set( | ||
| standardise_date(p) | ||
| for p in periods_to_exclude | ||
| if ( | ||
| standardise_date(p) >= lower_bound | ||
| and standardise_date(p) < upper_bound | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "def standardise_date|def standardise_datetime|def time_period_add" -A 15Repository: Flowminder/FlowKit
Length of output: 3015
🏁 Script executed:
sed -n '57,105p' flowmachine/flowmachine/utils.pyRepository: Flowminder/FlowKit
Length of output: 1241
🏁 Script executed:
sed -n '132,170p' flowmachine/flowmachine/utils.pyRepository: Flowminder/FlowKit
Length of output: 986
🏁 Script executed:
sed -n '70,100p' flowmachine/flowmachine/features/subscriber/total_locatable_periods.pyRepository: Flowminder/FlowKit
Length of output: 1339
🏁 Script executed:
rg -n "def parse_datestring" -A 20Repository: Flowminder/FlowKit
Length of output: 1344
🏁 Script executed:
sed -n '40,55p' flowmachine/flowmachine/utils.pyRepository: Flowminder/FlowKit
Length of output: 728
🏁 Script executed:
rg -n "periods_to_exclude" -B 5 -A 5 | head -60Repository: Flowminder/FlowKit
Length of output: 6833
Exclusions for hourly/minute periods may silently fail if provided as date-only strings.
The code normalises periods_to_exclude with standardise_date, which preserves time precision (HH:MM:SS). However, parse_datestring (called during normalisation) defaults date-only input like "2016-01-01" to "2016-01-01 00:00:00". For hourly or minute period_unit, period starts occur at specific times (e.g., "2016-01-01 14:30:00"), so date-only exclusions will not match and be silently ignored.
Either require periods_to_exclude to include explicit times matching the period granularity, document this constraint clearly, or enhance the filtering to handle date-only inputs intelligently based on period_unit.
🤖 Prompt for AI Agents
In `@flowmachine/flowmachine/features/subscriber/total_locatable_periods.py`
around lines 77 - 94, The exclusion filtering silently ignores date-only strings
for finer-grained periods because standardise_date (via parse_datestring)
defaults them to 00:00:00; update the filtering in the periods_to_exclude block
to treat date-only inputs as whole-day ranges for hour/minute period_unit: when
iterating periods_to_exclude, detect inputs that lack time (use parse_datestring
or original string heuristic), and if period_unit is 'hour' or 'minute' expand
that date into a range (start_of_day..end_of_day) and include any period start
that falls within that range (instead of only matching 00:00:00); keep using
standardise_date for datetime inputs, preserve the existing
lower_bound/upper_bound logic (time_period_add, total_periods, period_length),
and ensure final self.periods_to_exclude remains sorted and deduplicated as
before.
FlowAuth
|
||||||||||||||||||||||||||||
| Project |
FlowAuth
|
| Branch Review |
total-locatable-periods
|
| Run status |
|
| Run duration | 00m 47s |
| Commit |
|
| Committer | James Harrison |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7237 +/- ##
==========================================
- Coverage 92.09% 91.83% -0.27%
==========================================
Files 277 278 +1
Lines 10778 10808 +30
Branches 697 700 +3
==========================================
- Hits 9926 9925 -1
- Misses 700 730 +30
- Partials 152 153 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #7236
I have:
Description
Add
TotalLocatablePeriodsflowmachine query type - likeTotalActivePeriodsSubscriber, but only considering events at cells mapped to a specified spatial unit.Summary by CodeRabbit