Re-design time series management #349

daniel-thom · 2024-04-18T22:50:26Z

New features:

Allows addition of user-defined features to time series arrays.
Adds support for different time series resolutions.

Refactor/re-design:

Store time series metadata in a SQLite database instead of per-component dictionaries. This allows system-wide SQL queries instead of looping across component dictionaries.
Consolidate management of time series in TimeSeriesManager instead of individual time series storage implementations.

Features removed:

get_time_series and get_time_series_multiple no longer support abstract types. This could be restored, but I think it’s better this way. list_time_series* methods support abstract types.

- Store time series metadata in a SQLite database instead of per-component dictionaries. This allows system-wide SQL queries instead of looping across component dictionaries. - Consolidate management of time series in TimeSeriesManager instead of individual time series storage implementations. - Support addition of user-defined features to time series arrays. - Add support for different time series resolutions.

codecov · 2024-04-18T22:55:43Z

Codecov Report

Attention: Patch coverage is 92.61905% with 62 lines in your changes are missing coverage. Please review.

Project coverage is 75.61%. Comparing base (335a5a5) to head (5c89df2).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
+ Coverage   74.37%   75.61%   +1.24%     
==========================================
  Files          64       68       +4     
  Lines        4952     4876      -76     
==========================================
+ Hits         3683     3687       +4     
+ Misses       1269     1189      -80

Flag	Coverage Δ
unittests	`75.61% <92.61%> (+1.24%)`	⬆️

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

Files	Coverage Δ
src/InfrastructureSystems.jl	`80.00% <ø> (ø)`
src/abstract_time_series.jl	`85.71% <100.00%> (+10.71%)`	⬆️
src/component.jl	`91.66% <100.00%> (-3.60%)`	⬇️
src/containers.jl	`100.00% <100.00%> (ø)`
src/deterministic_metadata.jl	`95.45% <100.00%> (+86.36%)`	⬆️
src/probabilistic.jl	`81.66% <100.00%> (ø)`
src/scenarios.jl	`85.18% <100.00%> (ø)`
src/serialization.jl	`70.33% <ø> (+9.70%)`	⬆️
src/single_time_series.jl	`68.00% <100.00%> (ø)`
src/supplemental_attribute.jl	`94.44% <100.00%> (+13.59%)`	⬆️
... and 22 more

... and 3 files with indirect coverage changes

src/system_data.jl

test/test_time_series.jl

src/time_series_metadata_store.jl

src/hdf5_time_series_storage.jl

src/system_data.jl

daniel-thom · 2024-04-19T17:00:00Z

src/system_data.jl

+    time_series_type = time_series_type,
+)
+
+# TODO: do we need this? The old way of calculating this required a single resolution.


If we want this feature, it will have to be implemented in a different way.

jd-lara

I didn't see anything that looks problematic. Let's merge this as soon as as possible to make the PSI integration and open other PR's if further improvements are needed.

GabrielKS

Reviewed all but src/time_series_manager.jl and src/time_series_metadata_store.jl, I'll do those in a follow-up review. Marked some minor requests and questions.

src/InfrastructureSystems.jl

src/component.jl

src/descriptors/structs.json

src/hdf5_time_series_storage.jl

src/utils/print.jl

src/utils/sqlite.jl

src/utils/test.jl

test/test_time_series.jl

GabrielKS

Part 2: I have now reviewed src/time_series_manager.jl in its entirety and src/time_series_metadata_store.jl up to line 146. I'll pick it up again tomorrow.

src/time_series_manager.jl

GabrielKS · 2024-04-25T00:04:09Z

src/time_series_metadata_store.jl

+        "owner_category TEXT NOT NULL",
+        "features TEXT NOT NULL",
+        # The metadata is included as a convenience for serialization/de-serialization,
+        # specifically for types: time_series_type and scaling_factor_multplier.


Suggested change

# specifically for types: time_series_type and scaling_factor_multplier.

# specifically for types: time_series_type and scaling_factor_multiplier.

time_series_type is already a column though, right?

Its module is not. As it stands, we do not have columns for time_series_type's module, scaling_factor_multiplier, and the type and module for scaling_factor_multiplier. We could add them, guarantee that we will always have all columns for all metadata fields, and then remove this. Also, we would have to handle deserialization in a slightly more complicated way. Certainly possible.

src/time_series_metadata_store.jl

daniel-thom · 2024-04-25T14:20:19Z

src/time_series_metadata_store.jl

+        # The metadata is included as a convenience for serialization/de-serialization,
+        # specifically for types: time_series_type and scaling_factor_mulitplier.
+        # There is a lot duplication of data.
+        "metadata JSON NOT NULL",


I tested the length of this field for a time series with two features. 459 bytes. Here is likely a worst-case system: 100,000 components each with 10 time series arrays each with 2 features.

100_000 * 10 * 459 / (1024*1024) 437.73651123046875

437 MB wasted is likely not a big deal, but I'll prototype the alternative implementation just to see.

GabrielKS

I have now covered all the code.

test/test_time_series.jl

src/time_series_metadata_store.jl

src/time_series_manager.jl

src/time_series_metadata_store.jl

GabrielKS

I'm comfortable with the state of this now. Let's merge!

daniel-thom changed the title ~~feat(time-series): Re-design time series management~~ Re-design time series management Apr 18, 2024

feat(time-series): Code cleanup

68f0e04

github-actions bot reviewed Apr 19, 2024

View reviewed changes

src/system_data.jl Outdated Show resolved Hide resolved

test/test_time_series.jl Outdated Show resolved Hide resolved

test/test_time_series.jl Outdated Show resolved Hide resolved

feat(time-series): Code cleanup 2

1ba9637

daniel-thom force-pushed the dt/time-series-sqlite branch from 6ad6e31 to 1ba9637 Compare April 19, 2024 17:05

github-actions bot reviewed Apr 19, 2024

View reviewed changes

src/time_series_metadata_store.jl Outdated Show resolved Hide resolved

daniel-thom commented Apr 19, 2024

View reviewed changes

daniel-thom marked this pull request as ready for review April 19, 2024 17:20

daniel-thom requested review from jd-lara and GabrielKS April 19, 2024 17:21

daniel-thom mentioned this pull request Apr 19, 2024

Support time series redesign in InfrastructureSystems NREL-Sienna/PowerSystems.jl#1090

Merged

Remove invalid code

c33e8ba

daniel-thom force-pushed the dt/time-series-sqlite branch from fe417a8 to c33e8ba Compare April 19, 2024 23:28

Implement a SQLite backup function

c4c0bf8

jd-lara approved these changes Apr 22, 2024

View reviewed changes

jd-lara assigned daniel-thom Apr 22, 2024

daniel-thom added 3 commits April 22, 2024 17:13

Add missing file

7c80766

Merge branch 'main' into dt/time-series-sqlite

6e5e4c3

Standardize Base.summary for package types

80013e9

daniel-thom force-pushed the dt/time-series-sqlite branch from d99e02d to 80013e9 Compare April 23, 2024 19:56

GabrielKS requested changes Apr 24, 2024

View reviewed changes

daniel-thom added 6 commits April 24, 2024 18:15

Change the supertype of TimeSeriesData

29b8253

Remove unused file

1ed7baa

Fix time series range checks

c7f73e6

Restrict allowed types in time series features

93fa48d

Fix bug when building filter query with non-strings

1ff92bd

Code cleanup

b74317d

GabrielKS requested changes Apr 25, 2024

View reviewed changes

daniel-thom added 2 commits April 25, 2024 07:32

Fix indexes in SQL table

66f94fb

Add test of time_series_read_only

8e826af

daniel-thom commented Apr 25, 2024

View reviewed changes

Use JSONB format for metadata

b611c3b

GabrielKS requested changes Apr 25, 2024

View reviewed changes

This was referenced Apr 26, 2024

Need new interface for transform_single_time_series #350

Closed

Issues Deferred from Time Series Redesign #356

Open

daniel-thom added 2 commits April 26, 2024 11:27

Address PR comments

37b2e67

Fix bug with removing time series metadata

a9bd86b

GabrielKS requested changes Apr 26, 2024

View reviewed changes

src/time_series_metadata_store.jl Outdated Show resolved Hide resolved

Return nothing for forecast parameters if there are no forecasts

5c89df2

GabrielKS approved these changes Apr 26, 2024

View reviewed changes

daniel-thom merged commit e87f7b8 into main Apr 26, 2024
6 of 9 checks passed

daniel-thom deleted the dt/time-series-sqlite branch April 26, 2024 19:25

GabrielKS mentioned this pull request Jun 12, 2024

Deterministic Type Hierarchy #378

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-design time series management #349

Re-design time series management #349

daniel-thom commented Apr 18, 2024 •

edited

Loading

codecov bot commented Apr 18, 2024 •

edited

Loading

daniel-thom Apr 19, 2024

jd-lara left a comment

GabrielKS left a comment

GabrielKS left a comment

GabrielKS Apr 25, 2024

GabrielKS Apr 25, 2024

daniel-thom Apr 25, 2024

daniel-thom Apr 25, 2024

GabrielKS left a comment

GabrielKS left a comment

	# specifically for types: time_series_type and scaling_factor_multplier.
	# specifically for types: time_series_type and scaling_factor_multiplier.

Re-design time series management #349

Re-design time series management #349

Conversation

daniel-thom commented Apr 18, 2024 • edited Loading

codecov bot commented Apr 18, 2024 • edited Loading

Codecov Report

daniel-thom Apr 19, 2024

Choose a reason for hiding this comment

jd-lara left a comment

Choose a reason for hiding this comment

GabrielKS left a comment

Choose a reason for hiding this comment

GabrielKS left a comment

Choose a reason for hiding this comment

GabrielKS Apr 25, 2024

Choose a reason for hiding this comment

GabrielKS Apr 25, 2024

Choose a reason for hiding this comment

daniel-thom Apr 25, 2024

Choose a reason for hiding this comment

daniel-thom Apr 25, 2024

Choose a reason for hiding this comment

GabrielKS left a comment

Choose a reason for hiding this comment

GabrielKS left a comment

Choose a reason for hiding this comment

daniel-thom commented Apr 18, 2024 •

edited

Loading

codecov bot commented Apr 18, 2024 •

edited

Loading