-
Notifications
You must be signed in to change notification settings - Fork 8
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
Total PowerAnalytics Redesign: ComponentSelector
s and Metric
s
#24
Total PowerAnalytics Redesign: ComponentSelector
s and Metric
s
#24
Conversation
export mean, weighted_mean, unweighted_sum | ||
|
||
# IMPORTS | ||
import Base: @kwdef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in older minor versions we commit to supporting (see NREL-Sienna/InfrastructureSystems.jl#390)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. @jd-lara Is it time to force users to upgrade? 1.11 is out. If people haven't upgraded to at least 1.9, they probably should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import Base: @kwdef
is now standard in many Sienna packages, so if we want to make this decision we should make it uniformly across the ecosystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can brake the LTS support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a sense for how painful this would be for users: we could set the minimum version as the new LTS of 1.10.8 (https://julialang.org/downloads/#long_term_support_release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I propose we merge this with the import, which will make it consistent with PSY, IS, etc., and then do a bunch of "drop support pre-1.10" PRs to uniformly update things across the ecosystem.
This is the first commit in a major PowerAnalytics redesign.
Previously, the test simulations only contained one RenewableDispatch component and did not dispatch any thermal generation. Now there is a solar RenewableDispatch component, and nonzero thermal generation for 2/5 generators. The old PowerAnalytics test still work.
Additional changes in this commit: - Generalize compute_all to take Components - Make create_problem_results_dict return SortedDict
Rather than setting `agg_fn`/`reduce_fn` when calling `compute` or `aggregate_time`, metrics now specify how they should be aggregated across Entities and across time.
3173796
to
abd4467
Compare
@daniel-thom @jd-lara Ready for another review. If my resolutions above satisfy everyone, I think the outstanding items to discuss are:
Also, note that this depends on the unreleased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One minor comment you could consider.
src/builtin_component_selectors.jl
Outdated
# A subselector will be nothing if it doesn't fit under root_type | ||
subselectors = filter(!isnothing, subselectors) | ||
# Omit the category entirely if root_type causes elimination of all subselectors | ||
length(in_data[top_level]) > 0 && length(subselectors) == 0 && continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: isempty instead of length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/builtin_metrics.jl
Outdated
@@ -0,0 +1,374 @@ | |||
# TODO test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two TODOs done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now they are
src/output_utils.jl
Outdated
If the time axes match across all the `DataFrames`, horizontally concatenate them and remove | ||
the duplicate time axes. If not, throw an error | ||
""" | ||
function hcat_timed(vals::DataFrame...) # TODO incorporate allow_missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
See https://github.nrel.gov/gkonars/PowerAnalytics-demos/blob/main/new_pa_pr_demo.ipynb for a full demo. Depends on NREL-Sienna/InfrastructureSystems.jl#342 and NREL-Sienna/PowerSystems.jl#1197.