add interval and resolution to timeseries cache by jd-lara · Pull Request #573 · Sienna-Platform/InfrastructureSystems.jl

jd-lara · 2026-04-17T05:00:59Z

In order to use a single system in many models in PSI, the TimeSeriesCache keys needs more information to get the time series correctly

Copilot

Pull request overview

This PR extends time-series cache keying and lookup to support disambiguation when multiple time series share the same (type, name) but differ by resolution and/or interval, enabling reuse of a single system across multiple PSI models.

Changes:

Add resolution/interval parameters to get_time_series_uuid metadata lookup.
Extend TimeSeriesCacheKey to include resolution and interval.
Thread resolution through ForecastCache/StaticTimeSeriesCache construction and ensure cache updates request the same resolution/interval as the cached series.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
`src/time_series_interface.jl`	Adds `resolution` to UUID lookup via metadata to support disambiguation.
`src/time_series_cache.jl`	Extends cache key structure and propagates `resolution`/`interval` through cache constructors and update paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T05:07:22Z

    component_uuid::Base.UUID
    time_series_type::Type{<:TimeSeriesData}
    name::String
+    resolution::Dates.Period


TimeSeriesCacheKey now requires a non-null resolution::Dates.Period, but several public cache constructors accept resolution::Union{Nothing, Dates.Period} = nothing. This makes it impossible for callers to construct a cache key in the unambiguous case without first fetching metadata/time series to discover the actual resolution. Consider changing the field type to Union{Nothing, Dates.Period} (and treating nothing as “unspecified”), or add a helper/constructor that derives resolution/interval from metadata given (component, type, name) so callers can build stable keys without duplicating lookup logic.

Suggested change

resolution::Dates.Period

resolution::Union{Nothing, Dates.Period}

Copilot · 2026-04-17T05:07:23Z

@@ -237,13 +238,20 @@
    ::Type{T},
    component::InfrastructureSystemsComponent,
    name::AbstractString;
    start_time::Union{Nothing, Dates.DateTime} = nothing,
    horizon_count::Union{Nothing, Int} = nothing,
    cache_size_bytes = TIME_SERIES_CACHE_SIZE_BYTES,
    ignore_scaling_factors = false,
    interval::Union{Nothing, Dates.Period} = nothing,
+    resolution::Union{Nothing, Dates.Period} = nothing,
 ) where {T <: Forecast}


The ForecastCache docstring doesn’t mention the resolution keyword (and also omits interval), even though these keywords affect how the cached forecast is selected when multiple time series share the same (type, name). Please update the docstring argument list to document resolution (and interval if it’s intended to be part of the public API) so callers know how to disambiguate.

Copilot · 2026-04-17T05:07:23Z

    ::Type{T},
    component::InfrastructureSystemsComponent,
    name::AbstractString;
    start_time::Union{Nothing, Dates.DateTime} = nothing,
    horizon_count::Union{Nothing, Int} = nothing,
    cache_size_bytes = TIME_SERIES_CACHE_SIZE_BYTES,
    ignore_scaling_factors = false,
    interval::Union{Nothing, Dates.Period} = nothing,
+    resolution::Union{Nothing, Dates.Period} = nothing,
 ) where {T <: Forecast}
-    ts_metadata = get_time_series_metadata(T, component, name; interval = interval)
+    ts_metadata = get_time_series_metadata(
+        T,
+        component,
+        name;
+        resolution = resolution,
+        interval = interval,
+    )


ForecastCache now supports disambiguation via the new resolution keyword, but there isn’t a corresponding cache-level test case covering “multiple forecasts with the same name/type but different resolution” (analogous to the existing multiple-interval test). Adding a test that (1) builds two forecasts with different resolutions, (2) verifies ForecastCache(...; resolution=...) returns the expected data, and (3) verifies omitting resolution throws when ambiguous would prevent regressions.

Copilot · 2026-04-17T05:07:23Z

@@ -366,8 +378,9 @@
    cache_size_bytes = TIME_SERIES_CACHE_SIZE_BYTES,
    start_time::Union{Nothing, Dates.DateTime} = nothing,
    ignore_scaling_factors = false,
+    resolution::Union{Nothing, Dates.Period} = nothing,
 ) where {T <: StaticTimeSeries}
-    ts_metadata = get_time_series_metadata(T, component, name)
+    ts_metadata = get_time_series_metadata(T, component, name; resolution = resolution)
    initial_timestamp = get_initial_timestamp(ts_metadata)
    if start_time === nothing
        start_time = initial_timestamp
@@ -379,7 +392,14 @@
    end

    # Get an instance to assess data size.
-    ts = get_time_series(T, component, name; start_time = start_time, len = 1)
+    ts = get_time_series(
+        T,
+        component,
+        name;
+        start_time = start_time,
+        len = 1,
+        resolution = resolution,
+    )
    vals = get_time_series_values(component, ts; start_time = start_time, len = 1)


StaticTimeSeriesCache now threads the resolution keyword through to metadata lookup and get_time_series, but there’s no test covering the cache behavior when multiple SingleTimeSeries share the same (type, name) and differ only by resolution. Adding a test that constructs two resolutions and asserts (a) StaticTimeSeriesCache(...; resolution=...) reads the correct data and (b) omitting resolution throws on ambiguity would validate this new behavior.

Copilot · 2026-04-17T05:07:24Z

 function get_time_series_uuid(
    ::Type{T},
    component::InfrastructureSystemsComponent,
    name::AbstractString;
+    resolution::Union{Nothing, Dates.Period} = nothing,
    interval::Union{Nothing, Dates.Period} = nothing,
 ) where {T <: TimeSeriesData}
-    metadata = get_time_series_metadata(T, component, name; interval = interval)
+    metadata = get_time_series_metadata(
+        T,
+        component,
+        name;
+        resolution = resolution,
+        interval = interval,
+    )
    return get_time_series_uuid(metadata)


get_time_series_uuid now accepts resolution/interval to disambiguate metadata, but the existing tests only cover the single-resolution case. Adding a test that stores two time series with the same (type, name) but different resolution and asserts (1) calling without resolution throws on ambiguity and (2) calling with resolution returns the expected UUID would lock in the intended behavior.

codecov · 2026-04-17T05:10:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.90%. Comparing base (65ecc92) to head (0c59de8).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #573      +/-   ##
==========================================
+ Coverage   77.78%   78.90%   +1.12%     
==========================================
  Files          75       75              
  Lines        6733     6641      -92     
==========================================
+ Hits         5237     5240       +3     
+ Misses       1496     1401      -95

Flag	Coverage Δ
unittests	`78.90% <100.00%> (+1.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/time_series_cache.jl	`92.63% <100.00%> (+3.48%)`	⬆️
src/time_series_interface.jl	`96.24% <100.00%> (ø)`

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T06:38:53Z

 struct TimeSeriesCacheKey
    component_uuid::Base.UUID
    time_series_type::Type{<:TimeSeriesData}
    name::String
+    resolution::Dates.Period
+    interval::Union{Nothing, Dates.Period}
 end


TimeSeriesCacheKey now requires resolution and adds interval, which is a breaking change for any code constructing this key (including downstream packages). Consider adding an outer constructor (or deprecation shim) that preserves the previous 3-argument form and/or provides a helper to build the key from an existing cache/time series to avoid mismatches between the key fields and the selected metadata.

Copilot · 2026-04-17T06:38:53Z

+    resolution::Dates.Period
+    interval::Union{Nothing, Dates.Period}
 end



There are new semantics implied by adding resolution/interval to TimeSeriesCacheKey (it should now uniquely identify cached time series across multiple resolutions/intervals), but there is no unit test exercising key uniqueness (e.g., that two keys differing only by resolution/interval do not collide in a Dict). Adding a small regression test would help prevent future changes from reintroducing cache key collisions.

Suggested change

function Base.:(==)(x::TimeSeriesCacheKey, y::TimeSeriesCacheKey)

return x.component_uuid == y.component_uuid &&

x.time_series_type == y.time_series_type &&

x.name == y.name &&

x.resolution == y.resolution &&

x.interval == y.interval

end

Base.isequal(x::TimeSeriesCacheKey, y::TimeSeriesCacheKey) =

isequal(x.component_uuid, y.component_uuid) &&

isequal(x.time_series_type, y.time_series_type) &&

isequal(x.name, y.name) &&

isequal(x.resolution, y.resolution) &&

isequal(x.interval, y.interval)

function Base.hash(x::TimeSeriesCacheKey, h::UInt)

return hash(

(

x.component_uuid,

x.time_series_type,

x.name,

x.resolution,

x.interval,

),

h,

)

end

add interval and resolution to timeseries cache

800432e

jd-lara requested review from Copilot and daniel-thom April 17, 2026 05:00

Copilot started reviewing on behalf of jd-lara April 17, 2026 05:01 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

jd-lara added 2 commits April 16, 2026 23:37

fix docstring

682c082

improve testing

0c59de8

jd-lara requested a review from Copilot April 17, 2026 06:32

Copilot started reviewing on behalf of jd-lara April 17, 2026 06:33 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

daniel-thom approved these changes Apr 17, 2026

View reviewed changes

jd-lara merged commit 5d04c10 into main Apr 17, 2026
13 of 14 checks passed

jd-lara deleted the jd/add_resolution_to_cache branch April 20, 2026 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add interval and resolution to timeseries cache#573

add interval and resolution to timeseries cache#573
jd-lara merged 3 commits into
mainfrom
jd/add_resolution_to_cache

jd-lara commented Apr 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

codecov Bot commented Apr 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	resolution::Dates.Period
	resolution::Union{Nothing, Dates.Period}

+function Base.:(==)(x::TimeSeriesCacheKey, y::TimeSeriesCacheKey)
+    return x.component_uuid == y.component_uuid &&
+           x.time_series_type == y.time_series_type &&
+           x.name == y.name &&
+           x.resolution == y.resolution &&
+           x.interval == y.interval
+end
+Base.isequal(x::TimeSeriesCacheKey, y::TimeSeriesCacheKey) =
+    isequal(x.component_uuid, y.component_uuid) &&
+    isequal(x.time_series_type, y.time_series_type) &&
+    isequal(x.name, y.name) &&
+    isequal(x.resolution, y.resolution) &&
+    isequal(x.interval, y.interval)
+function Base.hash(x::TimeSeriesCacheKey, h::UInt)
+    return hash(
+        (
+            x.component_uuid,
+            x.time_series_type,
+            x.name,
+            x.resolution,
+            x.interval,
+        ),
+        h,
+    )
+end

Conversation

jd-lara commented Apr 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 17, 2026 •

edited

Loading