Add ComponentSelector feature: PSY portion (take two)
#1197
Conversation
- 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.
GabrielKS
force-pushed
the
gks/td/component_selector_port
branch
from
d2087ba to
1c65518
Compare
GabrielKS
changed the title
ComponentSelector feature: PSY portion (take two)
|
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. |
src/component_selector.jl
Outdated
| 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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
filter_func instead of filterby? Even if the second is a better term, it's better to keep consistency in the docstrings.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Renamed this to scope_limiter to try to reduce confusion — see this thread in the IS PR.
There was a problem hiding this comment.
scope_limiter is now no longer part of the user interface (see the IS PR).
src/component_selector.jl
Outdated
| if there is none. | ||
| - `filterby`: optional filter function to apply after evaluating the `ComponentSelector` | ||
| """ | ||
| get_component(selector::SingularComponentSelector, sys::System; filterby = nothing) = |
There was a problem hiding this comment.
Similar to above, these methods could be defined without kwargs or unions.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I was outvoted; the filterby kwarg is no more (see the IS PR)
| ::Type{T}, | ||
| sys::System; | ||
| subsystem_name = nothing, | ||
| ) where {T <: Component} | ||
| return IS.get_components(T, sys.data; subsystem_name = subsystem_name) | ||
| end | ||
|
|
||
| """ |
There was a problem hiding this comment.
Moved to get_components_interface (though there's an argument it should appear here too)
| @@ -1184,16 +1167,10 @@ function get_components_by_name( | |||
| return IS.get_components_by_name(T, sys.data, name) | |||
| end | |||
|
|
|||
| """ | |||
There was a problem hiding this comment.
Moved to get_components_interface (though there's an argument it should appear here too)
|
@daniel-thom Ready for another review. Besides the changes in the IS PR, the main new change here is discussed at the top of |
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.