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

Standardize Importing Conventions From IS #388

Open
GabrielKS opened this issue Jul 24, 2024 · 5 comments
Open

Standardize Importing Conventions From IS #388

GabrielKS opened this issue Jul 24, 2024 · 5 comments

Comments

@GabrielKS
Copy link
Contributor

GabrielKS commented Jul 24, 2024

Mostly not an issue with the IS codebase itself, but an issue with how all the other Sienna packages use IS. Since IS doesn't export anything, it is up to things that import it what identifiers they want to explicitly bring into their own namespace, and we are pretty inconsistent about this.

Screenshot 2024-07-24 at 10 47 18 AM

Sometimes, it has been argued, we might define a function in a downstream package that has the same name as a semantically similar function in IS because we only want the downstream function to be visible to the user, e.g., when they query for documentation — this is the reasoning for get_components.

This gets more complicated when there are multiple downstream packages that both define their own versions of a function rather than adding methods to the IS version. Here's a case where there are four different functions that are semantically all the same thing:
Screenshot 2024-07-24 at 10 33 35 AM
PSI isn't importing IS's version, but also PSI isn't importing PSY's version, so things get complicated.

That gets even worse when multiple downstream packages export two semantically identical functions with the same name:
Screenshot 2024-07-24 at 11 30 51 AM

When thinking through how to import things from IS.Optimization into PowerSimulations, I made a list of categories starting here, but I ended up missing a few functions, causing NREL-Sienna/PowerSimulations.jl#1125.

My hope is that at some point we can come up with some guidelines that rigorously define how things ought to be imported across the Sienna system and document them somewhere to avoid future issues like this.

@GabrielKS
Copy link
Contributor Author

GabrielKS commented Sep 26, 2024

The fact that IS.get_components !== PSY.get_components has made the ComponentSelector (#342, NREL-Sienna/PowerSystems.jl#1197) implementation less clean (see references here and here). The larger theme here — that it would be nice if system-like structs had a shared interface that included the same get_components function — in my opinion outweighs the concerns about confusing the user with extra methods.

@kdayday
Copy link
Contributor

kdayday commented Oct 23, 2024

This is also an issue for documentation consistency. I have just found that the IS version of get_components describes the filter_func argument, and the PSY version does not have an argument list, only selected examples -- so the filter function wasn't clearly stated as an option in PSY.

@GabrielKS
Copy link
Contributor Author

GabrielKS commented Oct 25, 2024

Just ran into another problem with this. I want to implement get_components on sets of results that forwards to the attached system, filtering to only available components when relevant. If get_components, get_available_components, etc. were the same function across IS, PSY, and PSI, the implementation plan would be straightforward:

  1. In IS, implement get_available_components that forwards to get_components for all system-like structs that have a get_components
  2. In PSY, implement get_available_components on System that filters get_components by get_available (this is already partially done)
  3. In IS, implement get_components on OptimizationProblemResults that forwards to get_available_components of get_source_data
  4. In PSI, implement get_components on SimulationProblemResults that forwards to get_available_components of get_system

In the status quo, this is far more complicated: if OptimizationProblemResults.source_data is an IS type, we need to use IS.get_components, but if it's a PSY type, we need to use PSY.get_components. I can't think of a non-hacky way to do this if those are not the same function. Maybe the least worst option is to only define get_components(..., ::OptimizationProblemResults) in PSY or PSI and only have it work for PSY type source_data, which is inflexible and (adjacent to?) type piracy.

@GabrielKS
Copy link
Contributor Author

I am now officially in support of making PSY.get_components === IS.get_components and the same for all related functions (get_component, get_available_components, etc.); I'd be happy to do the implementation of this.

@kdayday
Copy link
Contributor

kdayday commented Oct 25, 2024

@GabrielKS , relatedly, I am pushing documentation updates for get_components and get_available_components in PSY -- would be great if those could be incorporated wherever you do this merge. NREL-Sienna/PowerSystems.jl#1206

GabrielKS added a commit to NREL-Sienna/PowerSystems.jl that referenced this issue Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants