Add `ComponentSelector` feature: IS portion #342

GabrielKS · 2024-03-18T23:51:43Z

~~This draft PR will get filled in as @tengis-nrl and I move the ComponentSelector features from PowerAnalytics to InfrastructureSystems.~~ Ready for review! 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.

src/component_selector.jl

codecov · 2024-03-24T07:21:22Z

Codecov Report

Attention: Patch coverage is 90.16393% with 12 lines in your changes missing coverage. Please review.

Project coverage is 78.30%. Comparing base (be8eb50) to head (dd3bba2).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
src/results.jl	0.00%	9 Missing ⚠️
src/time_series_storage.jl	0.00%	2 Missing ⚠️
src/component_container.jl	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #342      +/-   ##
==========================================
+ Coverage   78.28%   78.30%   +0.02%     
==========================================
  Files          71       71              
  Lines        5484     5596     +112     
==========================================
+ Hits         4293     4382      +89     
- Misses       1191     1214      +23

Flag	Coverage Δ
unittests	`78.30% <90.16%> (+0.02%)`	⬆️

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

Files with missing lines	Coverage Δ
src/InfrastructureSystems.jl	`55.55% <ø> (-34.45%)`	⬇️
src/Optimization/optimization_container_keys.jl	`95.58% <100.00%> (ø)`
src/common.jl	`66.66% <ø> (+6.66%)`	⬆️
src/component_selector.jl	`100.00% <100.00%> (ø)`
src/components.jl	`86.39% <ø> (ø)`
src/system_data.jl	`91.30% <100.00%> (+0.02%)`	⬆️
src/component_container.jl	`92.85% <92.85%> (ø)`
src/time_series_storage.jl	`61.53% <0.00%> (ø)`
src/results.jl	`0.00% <0.00%> (ø)`

... and 13 files with indirect coverage changes

tengis-nrl · 2024-03-25T16:41:30Z

jl_5D19.tmp

GabrielKS · 2024-03-27T15:25:12Z

We need working implementations of get_components and get_subselectors here, not just in PowerSystems, because the ComponentSelector feature is meant to be usable without PowerSystems. You'll note a TODO here about making get_available optional; if you don't think we have time to do that now, I think the IS version of get_components should just not check availability and the PSY version should.

GabrielKS · 2024-06-10T18:46:15Z

Rebased onto main and tests pass.

src/component_selector.jl

daniel-thom · 2024-10-08T20:34:38Z

src/component_selector.jl

+"""
+make_selector(
+    component_type::Type{<:InfrastructureSystemsComponent},
+    filter_func::Function;


Shouldn't this be first?

There is a signature (line 386) where it is first. Is this compromise where I have both signatures acceptable? It just seems really inconsistent to me to have the user specify the component type first in every case except the filter function case. This way the do block is supported but I also get my consistency.

My concern is consistency in our API. Currently, there is only one way to call get_components with a filter function. That function has to be first. This PR is introducing a second pattern where you can call get_components and specify the filter function or scope limiter as a keyword argument. If people see this pattern being used they may try to use it with the existing get_components and then complain that it doesn't work. I don't want to have to then go back and change that method to support this new pattern. My suggestion is that we continue to provide a single way to specify filter functions - and that it follow the Julia convention.

For the record, @daniel-thom, @jd-lara, @sourabhdalvi, @akrivi and I discussed this synchronously and came to the following resolution:

Eliminate the scope_limiter feature. Instead:

When the user calls get_components(selector, system), return all components that satisfy the selector (no availability filtering)

When the user calls get_components(selector, results), return only the available components

When the user calls get_available_components(selector, system), return only the available components

Follow the Julia convention of always putting the function first; remove the methods where the function is not first (I was outvoted)

From the user's point of view, this is done; all function parameters should only appear in first position. There is a hidden scope_limiter kwarg to aid in implementation and extension.

This reverts commit 15b3f31.

GabrielKS · 2024-12-12T19:25:11Z

src/component_container.jl

+abstract type ComponentContainer <: InfrastructureSystemsContainer end
+
+# For each of these functions, `ComponentContainer` concrete subtypes MUST either implement
+# appropriate methods or accept the default if it exists.


@daniel-thom these methods are necessary and sufficient for the ComponentSelector design and I'm trying to avoid further scope creep. But maybe we also want iterate_components, etc. here?

Mabye. I'm definitely OK leaving it out.

GabrielKS · 2024-12-12T19:29:28Z

src/system_data.jl

@@ -17,7 +17,7 @@ const SERIALIZATION_METADATA_KEY = "__serialization_metadata__"

 Container for system components and time series data
 """
-mutable struct SystemData <: InfrastructureSystemsType
+mutable struct SystemData <: ComponentContainer


ComponentContainer <: InfrastructureSystemsType so this is backwards compatible

GabrielKS · 2024-12-12T20:35:50Z

@daniel-thom Ready for another review. The main changes since last review are:

Cleanup of the scope_limiter situation. More details above, but the gist is that this is no longer part of the user interface, its functionality is served by get_components vs. get_available_components.
Addition of rebuild_selector and the supporting type RegroupedComponentSelector to fulfill a request from @akrivi
Addition of ComponentContainer with a standard interface for get_components-type stuff

daniel-thom · 2024-12-16T16:23:46Z

src/component_selector.jl

+function get_components(selector::FilterComponentSelector, sys; kwargs...)
+    # Short-circuit-evaluate the `scope_limiter` first so `filter_func` may refer to
+    # component attributes that do not exist in components outside the scope
+    scope_limiter = get(kwargs, :scope_limiter, nothing)


It seems like you want scope_limiter to be part of the public interface, or at least you may be setting it in PowerAnalytics. Is that correct?

I don't want it to be part of the public interface, but it's used in the implementation of get_available_components, so I tried to strike a middle ground here with the undocumented kwarg (similar to your use_system_fallback kwarg)

daniel-thom · 2024-12-16T16:26:12Z

src/component_selector.jl

+Get the available components of the collection that make up the `ComponentSelector`.
+"""
+get_available_components(selector::ComponentSelector, sys) =
+    get_components(selector, sys; scope_limiter = x -> get_available(sys, x))


I'm a bit concerned that people will use this as example code. And then eventually ask why scope_limiter isn't a kwarg in the System get_components method. Wouldn't it be better to follow the Julia convention and pass the "scope_limiter" as the first argument?

The goals with this design are to (a) keep scope_limiter completely out of the public interface, as we resolved earlier, while (b) keeping it in a sort of "developer interface" so that it can be used for get_available_components. Absent another layer of indirection (have each ComponentSelector subtype implement _get_components with a scope_limiter and have get_components and get_available_components call that), putting scope_limiter as the first argument would more or less force it be part of the public interface: it would explicitly appear in method lists and such. Do you think it's worth it to add that layer of indirection, or is the current solution (perhaps with a comment added here that users are not to take this source code as example code?) good enough?

github-actions bot reviewed Mar 24, 2024

View reviewed changes

src/component_selector.jl Outdated Show resolved Hide resolved

src/component_selector.jl Outdated Show resolved Hide resolved

src/component_selector.jl Show resolved Hide resolved

tengis-nrl marked this pull request as ready for review March 25, 2024 16:22

jd-lara reviewed Mar 26, 2024

View reviewed changes

jl_5D19.tmp Outdated Show resolved Hide resolved

GabrielKS assigned tengis-nrl Mar 27, 2024

jd-lara assigned jd-lara and unassigned tengis-nrl Apr 22, 2024

GabrielKS changed the base branch from gks/td/component_selector_feature to main June 4, 2024 17:53

GabrielKS force-pushed the gks/td/component_selector_port branch from 1732638 to 83ea408 Compare June 10, 2024 18:44

GabrielKS assigned GabrielKS and unassigned jd-lara Jun 10, 2024

GabrielKS requested a review from daniel-thom June 10, 2024 23:50

GabrielKS mentioned this pull request Jun 11, 2024

Move ComponentSelector from PowerAnalytics to InfrastructureSystems: PA portion GabrielKS/PowerAnalytics.jl#2

Merged

GabrielKS force-pushed the gks/td/component_selector_port branch from f4e395d to d4c3aff Compare June 26, 2024 21:43

GabrielKS force-pushed the gks/td/component_selector_port branch 2 times, most recently from 0344a74 to 915d247 Compare July 17, 2024 21:08

GabrielKS force-pushed the gks/td/component_selector_port branch from 915d247 to b11ccf6 Compare September 18, 2024 20:49

GabrielKS mentioned this pull request Sep 18, 2024

Add ComponentSelector feature: PSY portion (take two) NREL-Sienna/PowerSystems.jl#1197

Open

GabrielKS and others added 9 commits September 25, 2024 16:35

Scaffold ComponentSelector move from PA, copy over implementation

7fda0b5

Move TopologyComponentSelector from IS to PSY

3999abe

Make code work - WIP

32eb399

implement component selector

f0314e9

make PA tests pass

0b34636

run formatter

2d5445e

remove tmp files

bf907e4

Remove temp file

7ee4018

ComponentSelector: Restore getters to IS, refurbish docs

a3a09a9

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

Open

GabrielKS added 3 commits October 2, 2024 16:36

Address PR comments: rename filterby -> scope_limiter, etc.

3eed03a

ComponentSelector: validate in constructors, eliminate default_name

79cfd57

Make get_components always return FlattenIteratorWrapper

e27f25a

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

daniel-thom reviewed Oct 8, 2024

View reviewed changes

GabrielKS added 15 commits October 25, 2024 16:04

Implement filter(..., ::IS.FlattenIteratorWrapper)

15b3f31

Revert "Implement filter(..., ::IS.FlattenIteratorWrapper)"

ebe306d

This reverts commit 15b3f31.

ComponentSelector: Eliminate scope_limiter from public interface

22e33e4

Require filter function as first argument in make_selector

b29ce9e

Make groupby default to :each instead of :all

9b24629

Implement rebuild_selector: easy cases

62c0de4

Add RegroupedComponentSelector to regroup ListComponentSelector

ed5c391

Add get_available... methods in IS

ccdab57

Add SystemLike abstract type

15497ea

Rename SystemData to ComponentContainer

b6dc5d2

Centralize implementations of 'availability' in IS

b579a5c

Implement get_component(s) for IS.Results

ae66eb4

Add InfrastructureSystemsContainer methods on ComponentContainer

0ecfb06

ComponentSelector: Clean up default name code

0edc62b

Remove duplicate get_component(s) definitions

1a70ca6

GabrielKS commented Dec 12, 2024

View reviewed changes

Move ComponentContainer tests to their own file

dd3bba2

GabrielKS mentioned this pull request Dec 12, 2024

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

Open

GabrielKS requested a review from daniel-thom December 12, 2024 20:35

daniel-thom reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ComponentSelector` feature: IS portion #342

Add `ComponentSelector` feature: IS portion #342

GabrielKS commented Mar 18, 2024 •

edited

Loading

codecov bot commented Mar 24, 2024 •

edited

Loading

tengis-nrl commented Mar 25, 2024

GabrielKS commented Mar 27, 2024

GabrielKS commented Jun 10, 2024

daniel-thom Oct 8, 2024

GabrielKS Oct 9, 2024

daniel-thom Oct 9, 2024

GabrielKS Oct 10, 2024

GabrielKS Dec 12, 2024 •

edited

Loading

GabrielKS Dec 12, 2024 •

edited

Loading

daniel-thom Dec 16, 2024

GabrielKS Dec 12, 2024

GabrielKS commented Dec 12, 2024 •

edited

Loading

daniel-thom Dec 16, 2024

GabrielKS Dec 16, 2024 •

edited

Loading

daniel-thom Dec 16, 2024

GabrielKS Dec 16, 2024 •

edited

Loading

Add ComponentSelector feature: IS portion #342

Are you sure you want to change the base?

Add ComponentSelector feature: IS portion #342

Conversation

GabrielKS commented Mar 18, 2024 • edited Loading

codecov bot commented Mar 24, 2024 • edited Loading

Codecov Report

tengis-nrl commented Mar 25, 2024

GabrielKS commented Mar 27, 2024

GabrielKS commented Jun 10, 2024

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 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GabrielKS commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

GabrielKS Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GabrielKS Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Add `ComponentSelector` feature: IS portion #342

Add `ComponentSelector` feature: IS portion #342

GabrielKS commented Mar 18, 2024 •

edited

Loading

codecov bot commented Mar 24, 2024 •

edited

Loading

GabrielKS Dec 12, 2024 •

edited

Loading

GabrielKS Dec 12, 2024 •

edited

Loading

GabrielKS commented Dec 12, 2024 •

edited

Loading

GabrielKS Dec 16, 2024 •

edited

Loading

GabrielKS Dec 16, 2024 •

edited

Loading