Add `ComponentSelector` feature: PSY portion (take two) #1197

GabrielKS · 2024-09-18T20:57:42Z

Successor to #1079 and companion to/depends on NREL-Sienna/InfrastructureSystems.jl#342. Adds an immutable, lazy, system-independent representation of a grouped set of components. For more details and a demonstration, see https://github.nrel.gov/gkonars/PowerAnalytics-demos/blob/main/component_selector_pr_demo.ipynb.

GabrielKS · 2024-09-26T00:31:33Z

This is now ready for in-depth code review! Note the dependency on NREL-Sienna/InfrastructureSystems.jl#342. For a demonstration, see https://github.nrel.gov/gkonars/PowerAnalytics-demos/blob/main/component_selector_pr_demo.ipynb.

daniel-thom · 2024-09-30T18:33:36Z

src/component_selector.jl

+get_components(selector::ComponentSelector, sys::System; filterby = nothing) =
+    IS.get_components(selector, sys; filterby = filterby)
+
+"""
+    get_components(filterby, selector, sys)
+Get the components of the `System` that make up the `ComponentSelector`.
+ - `filterby`: optional filter function to apply after evaluating the `ComponentSelector`
+"""
+get_components(
+    filterby::Union{Nothing, Function},
+    selector::ComponentSelector,
+    sys::System,
+) =
+    get_components(selector, sys; filterby = filterby)


I think this would be slightly cleaner.

get_components(selector::ComponentSelector, sys::System) = IS.get_components(selector, sys) get_components(filterby::Function, selector::ComponentSelector, sys::System) = IS.get_components(selector, sys; filterby = filterby)

filter_func instead of filterby? Even if the second is a better term, it's better to keep consistency in the docstrings.

Related to our offline discussion, the fact that you can create the component selector with a filter function and then pass a separate filter function here adds complexity and possibly confusion to the user interface.

Renamed this to scope_limiter to try to reduce confusion — see this thread in the IS PR.

scope_limiter is now no longer part of the user interface (see the IS PR).

daniel-thom · 2024-09-30T18:43:18Z

src/component_selector.jl

+if there is none.
+ - `filterby`: optional filter function to apply after evaluating the `ComponentSelector`
+"""
+get_component(selector::SingularComponentSelector, sys::System; filterby = nothing) =


Similar to above, these methods could be defined without kwargs or unions.

I'm fine to add the method where the function is the first argument, but I'm hoping to keep the kwarg version as well — it makes more sense to me in the context where this would be used. Is that objectionable?

I was outvoted; the filterby kwarg is no more (see the IS PR)

src/base.jl

GabrielKS · 2024-12-12T20:22:13Z

src/base.jl

@@ -1184,16 +1167,10 @@ function get_components_by_name(
    return IS.get_components_by_name(T, sys.data, name)
 end

-"""


Moved to get_components_interface (though there's an argument that it should appear here too)

GabrielKS · 2024-12-12T20:36:59Z

@daniel-thom Ready for another review. Besides the changes in the IS PR, the main new change here is discussed at the top of get_components_interface.jl.

jd-lara · 2025-01-08T19:02:40Z

@GabrielKS we will scope this PR for PSY 5.0

- Originally, all `ComponentSelector`s always filtered out components marked not available. This is what we want in `PowerAnalytics`, but not necessarily what is wanted if `ComponentSelector` is used elsewhere. The solution is to not automatically perform this filtering but add it as an option so that `PowerAnalytics` can still have it. - This commit also defines the behavior that using a `TopologyComponentSelector` on a system that does not have that topology element returns no components/subselectors.

...related to NREL-Sienna/InfrastructureSystems.jl#388

daniel-thom · 2025-01-18T18:21:58Z

src/get_components_interface.jl

+
+# get_groups
+"""
+Get the groups that make up the `ComponentSelector`. Optionally specify a filter function


The functionality in get_groups combined with groupby is very powerful and clever. However, the documentation is a bit lacking.

I understand how these capabilities are used in PowerAnalytics.jl. Let's take an example where you want to sum active_power for generators grouped by fuel type. We need this functionality merged ASAP. 100% agree and let's do it.

If this is to be a public interface that we expect users to use, the documentation needs improvement. At minimum, there should be a complete workflow example. Common questions will be, "what is the difference between groupby = :each and groupby = :all? What is an example of a function that should be passed? This leaves me with three conflicting opinions. (1) Leave it alone with the understanding that very few people will ever consider this. (2) Don't export this method. (3) Go all in and give complete documentation.

Once a power user understands what's going on here, they are likely to ask why we did all this. They might say, why not rely on existing packages, like this example

using DataFrames julia> combine(groupby(DataFrame(get_components(ThermalStandard, sys)), :fuel), :active_power => sum) 5×2 DataFrame Row │ fuel active_power_sum │ ThermalF… Float64 ─────┼───────────────────────────────────────────────────── 1 │ ThermalFuels.NATURAL_GAS = 7 29.7472 2 │ ThermalFuels.COAL = 1 13.6381 3 │ ThermalFuels.DISTILLATE_FUEL_OIL… 6.5 4 │ ThermalFuels.OTHER = 14 0.0 5 │ ThermalFuels.NUCLEAR = 9 0.849257

If we go all in on documentation, we should anticipate this question and address it.

daniel-thom

I think it's ready to merge unless @jd-lara or @GabrielKS want to change something about public exports. Documentation improvements can be made in future work.

GabrielKS · 2025-01-18T19:08:20Z

@daniel-thom I definitely agree that documentation needs elaboration (and that that should be done in a subsequent PR). I'm good with the public exports as they are right now if others are.

GabrielKS force-pushed the gks/td/component_selector_port branch from d2087ba to 1c65518 Compare September 25, 2024 22:37

GabrielKS changed the title ~~Move ComponentSelector from PowerAnalytics to InfrastructureSystems: PSY portion (take two)~~ Add ComponentSelector feature: PSY portion (take two) Sep 26, 2024

GabrielKS mentioned this pull request Sep 26, 2024

Standardize Importing Conventions From IS NREL-Sienna/InfrastructureSystems.jl#388

Open

GabrielKS marked this pull request as ready for review September 26, 2024 00:31

GabrielKS requested review from daniel-thom and jd-lara September 26, 2024 00:31

GabrielKS self-assigned this Sep 27, 2024

daniel-thom reviewed Sep 30, 2024

View reviewed changes

This was referenced Oct 1, 2024

Feature: allow the use of ComponentSelector to filter devices to model NREL-Sienna/PowerSimulations.jl#1152

Open

Total PowerAnalytics Redesign: ComponentSelectors and Metrics NREL-Sienna/PowerAnalytics.jl#24

Merged

GabrielKS requested a review from daniel-thom October 4, 2024 17:44

GabrielKS commented Dec 12, 2024

View reviewed changes

src/base.jl Show resolved Hide resolved

GabrielKS commented Dec 12, 2024

View reviewed changes

GabrielKS mentioned this pull request Dec 12, 2024

Add ComponentSelector feature: PSI portion NREL-Sienna/PowerSimulations.jl#1196

Merged

GabrielKS force-pushed the gks/td/component_selector_port branch from f0de327 to 01c1146 Compare January 15, 2025 01:56

GabrielKS and others added 11 commits January 17, 2025 13:16

Scaffold ComponentSelector move from PA, copy over tests

dbe06a0

Move TopologyComponentSelector from IS to PSY

b43766f

make tests pass - WIP

14f3bbd

make component selector tests pass

2d0f0af

make PA tests pass

9114aaa

make PSY tests pass

8ce7301

ComponentSelector: move implementations to IS, add wrappers here

443e6c6

Reinstate component_to_qualified_string

f0b95cb

Remove TODO

596659e

ComponentSelector: Implement in(component, selector, [system])

13c5643

GabrielKS added 18 commits January 17, 2025 13:18

Address PR comments: rename subtype -> type, etc.

a61aa67

Address PR comments: add function-first interfaces

6c835d4

Address PR comments: rename filterby -> scope_limiter

29bf845

ComponentSelector: validate in constructors, rewrap

fcc5866

Make get_components always return FlattenIteratorWrapper

883a202

Build out get_available_components, get_available_component

65f87fc

Implement get_available_groups, add tests

2d34724

ComponentSelector: Eliminate scope_limiter from public interface

e4078f3

Remove public scope_limiter; require filter first in make_selector

873f493

Make groupby default to :each instead of :all

134b55e

Implement rebuild_selector: easy cases

a194af7

Support more regrouping, uncover get_components bug...

681d1a6

...related to NREL-Sienna/InfrastructureSystems.jl#388

Redesign IS to PSY handoff of get_components-like functions

c569aad

Use IS.SystemLike

fde4f51

Use new IS implementations of availability, misc cleanup

006dd48

Add Base.show(::IO, ::System)

f8acd54

Remove temporary kludge

55ff080

ComponentSelector: make scope_limiter an optional first argument

3332ff4

GabrielKS force-pushed the gks/td/component_selector_port branch from 863f4d8 to 3332ff4 Compare January 17, 2025 20:18

GabrielKS added 3 commits January 17, 2025 16:43

Include new files in docs, remove signatures, add crucial docstring

474e6be

Restore lost get_components docstrings

de4ffd9

Improve ComponentSelector documentation

4e70698

daniel-thom reviewed Jan 18, 2025

View reviewed changes

daniel-thom approved these changes Jan 18, 2025

View reviewed changes

jd-lara mentioned this pull request Jan 19, 2025

get_groups documentation #1256

Closed

jd-lara approved these changes Jan 19, 2025

View reviewed changes

jd-lara merged commit 21160e7 into main Jan 19, 2025
1 of 8 checks passed

jd-lara deleted the gks/td/component_selector_port branch January 19, 2025 06:50

GabrielKS mentioned this pull request Feb 12, 2025

Complete ComponentSelector docstrings #1283

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ComponentSelector` feature: PSY portion (take two) #1197

Add `ComponentSelector` feature: PSY portion (take two) #1197

GabrielKS commented Sep 18, 2024 •

edited

Loading

GabrielKS commented Sep 26, 2024

daniel-thom Sep 30, 2024 •

edited

Loading

daniel-thom Sep 30, 2024

daniel-thom Sep 30, 2024

GabrielKS Oct 2, 2024

GabrielKS Dec 12, 2024

daniel-thom Sep 30, 2024

GabrielKS Oct 4, 2024

GabrielKS Dec 12, 2024

GabrielKS Dec 12, 2024 •

edited

Loading

GabrielKS commented Dec 12, 2024 •

edited

Loading

jd-lara commented Jan 8, 2025

daniel-thom Jan 18, 2025

daniel-thom left a comment

GabrielKS commented Jan 18, 2025

Add ComponentSelector feature: PSY portion (take two) #1197

Add ComponentSelector feature: PSY portion (take two) #1197

Conversation

GabrielKS commented Sep 18, 2024 • edited Loading

GabrielKS commented Sep 26, 2024

daniel-thom Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GabrielKS Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

GabrielKS commented Dec 12, 2024 • edited Loading

jd-lara commented Jan 8, 2025

Choose a reason for hiding this comment

daniel-thom left a comment

Choose a reason for hiding this comment

GabrielKS commented Jan 18, 2025

Add `ComponentSelector` feature: PSY portion (take two) #1197

Add `ComponentSelector` feature: PSY portion (take two) #1197

GabrielKS commented Sep 18, 2024 •

edited

Loading

daniel-thom Sep 30, 2024 •

edited

Loading

GabrielKS Dec 12, 2024 •

edited

Loading

GabrielKS commented Dec 12, 2024 •

edited

Loading