Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jd/add thermal time series #1162

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Jd/add thermal time series #1162

wants to merge 11 commits into from

Conversation

jd-lara
Copy link
Member

@jd-lara jd-lara commented Oct 13, 2024

closes #1153 partially. Still missing situations when this is mixed with dispatch models.

If the approach in this PR is ok, I will implement for dispatch models and two stage simulations.

@jd-lara jd-lara self-assigned this Oct 13, 2024
Copy link
Contributor

github-actions bot commented Oct 13, 2024

Performance Results

Version Precompile Time
Main 4.394310773
This Branch 4.489718702
Version Build Time
Main-Build Time Precompile 67.457353893
Main-Build Time Postcompile 1.014952664
This Branch-Build Time Precompile 69.518155119
This Branch-Build Time Postcompile 1.095375151
Version Build Time
Main-Solve Time Precompile 1195.303225806
Main-Solve Time Postcompile 1160.332067219
This Branch-Solve Time Precompile 873.019986163
This Branch-Solve Time Postcompile 835.260607043

Copy link

codecov bot commented Oct 13, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 27 lines in your changes missing coverage. Please review.

Project coverage is 76.30%. Comparing base (5863e9e) to head (fc6fc96).

Files with missing lines Patch % Lines
...vice_constructors/thermalgeneration_constructor.jl 55.00% 18 Missing ⚠️
.../devices_models/devices/common/range_constraint.jl 33.33% 8 Missing ⚠️
...rc/parameters/update_container_parameter_values.jl 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1162      +/-   ##
==========================================
- Coverage   76.40%   76.30%   -0.10%     
==========================================
  Files         121      121              
  Lines       13089    13149      +60     
==========================================
+ Hits        10000    10034      +34     
- Misses       3089     3115      +26     
Flag Coverage Δ
unittests 76.30% <57.14%> (-0.10%) ⬇️

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

Files with missing lines Coverage Δ
src/PowerSimulations.jl 100.00% <ø> (ø)
...evices_models/devices/default_interface_methods.jl 35.71% <ø> (ø)
src/devices_models/devices/thermal_generation.jl 84.71% <100.00%> (+0.11%) ⬆️
src/parameters/add_parameters.jl 74.20% <100.00%> (+0.47%) ⬆️
src/services_models/services_constructor.jl 97.60% <100.00%> (ø)
...rc/parameters/update_container_parameter_values.jl 34.90% <50.00%> (+0.14%) ⬆️
.../devices_models/devices/common/range_constraint.jl 73.06% <33.33%> (-1.65%) ⬇️
...vice_constructors/thermalgeneration_constructor.jl 94.49% <55.00%> (-3.82%) ⬇️

@jd-lara
Copy link
Member Author

jd-lara commented Oct 13, 2024

@rodrigomha and @sourabhdalvi take a look at the branch. I think this can benefit from more testing but this approach should suffice.

@daniel-thom Can you confirm the use of has_time_series is adequate in this context? If this approach works we can implement for other situations where not all the devices might have time series and save the users the need to define vectors of 1.0's

@jd-lara jd-lara marked this pull request as ready for review October 13, 2024 23:58
@daniel-thom
Copy link
Contributor

daniel-thom commented Oct 14, 2024

@rodrigomha and @sourabhdalvi take a look at the branch. I think this can benefit from more testing but this approach should suffice.

@daniel-thom Can you confirm the use of has_time_series is adequate in this context? If this approach works we can implement for other situations where not all the devices might have time series and save the users the need to define vectors of 1.0's

I expect that this is substantially slower than the PSY 3.0 version because each call to has_time_series corresponds to one SQL query. It's probably the difference between nanoseconds and microseconds/low-milliseconds. If that is concerning, we could make a new function to get components with time series that makes a single SQL query. It could work with either get_components or an existing iterable of components.

This depends on how many times the function will be called (how many components).

julia> length(get_components(Component, sys))
525

julia> @time collect(get_components(x -> has_time_series(x), Component, sys));
  0.271341 seconds (95.72 k allocations: 6.984 MiB, 74.16% compilation time)

julia> @time has_time_series(g)
  0.002146 seconds (31 allocations: 1.922 KiB)

@jd-lara
Copy link
Member Author

jd-lara commented Oct 14, 2024

@rodrigomha and @sourabhdalvi take a look at the branch. I think this can benefit from more testing but this approach should suffice.
@daniel-thom Can you confirm the use of has_time_series is adequate in this context? If this approach works we can implement for other situations where not all the devices might have time series and save the users the need to define vectors of 1.0's

I expect that this is substantially slower than the PSY 3.0 version because each call to has_time_series corresponds to one SQL query. It's probably the difference between nanoseconds and microseconds/low-milliseconds. If that is concerning, we could make a new function to get components with time series that makes a single SQL query. It could work with either get_components or an existing iterable of components.

This depends on how many times the function will be called (how many components).

julia> length(get_components(Component, sys))
525

julia> @time collect(get_components(x -> has_time_series(x), Component, sys));
  0.271341 seconds (95.72 k allocations: 6.984 MiB, 74.16% compilation time)

julia> @time has_time_series(g)
  0.002146 seconds (31 allocations: 1.922 KiB)

We need to figure this out because this is critical for NTP and other systems and we can't have this kind of performance hit since this is called multiple times in multiple places. We ought to put some other way to compute this property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add max_active_power time series to thermal standard
2 participants