Conversation
coroa
left a comment
There was a problem hiding this comment.
Puh that thing is huge, i only covered a small bit of the full amount of code. Most of it is looking good, but well alot.
pypsa/definitions/components.py
Outdated
| ) | ||
| return pd.DataFrame(active).any(axis=1) & self.static.active | ||
| for key, value in super().items(): | ||
| if key != "Network": # TODO Drop Network completly? |
There was a problem hiding this comment.
Why is the Network even in the componentstore. In my understanding Network is no component. For instance what are its static or dynamic attributes?
There was a problem hiding this comment.
Good question. Added by Tom 7 years ago to the components.csv and was never touched since then. It is removed now. If there are people who rely on n.components.Network, this will break now.
| ['Generator class instance', 'Storage class instance'] | ||
| """ | ||
| if isinstance(item, (list, set)): | ||
| return [self[key] for key in item] |
There was a problem hiding this comment.
For consistency with DataFrame it might make sense to wrap this in a new ComponentsStore instance.
There was a problem hiding this comment.
Hmm. I see, but not sure if I agree. For DataFrame consistency it makes sense and for them it also is useful since you wanna use DataFrame specific features on your sliced df.
But here there is no use case where you would need any ComponentsStore functionality on a sliced version. And then it just complicates things for no reason. If this returns a list, the ComponentsStore is always a complete representation of all components for a specific Network.
|
I added the missing features for this PR (custom component class workflow and better alias access) and resolved the existing review comments.
Yeah, it got a bit too big, I agree. Sorry. @FabianHofmann @coroa If you don't have any bigger structural concerns, I would say let's merge this thing. Maybe after another release with the statistic expressions. I will go through some tests (add pytests, performance, pypsa-eur) but if they all work, the actual user impact should be quite minimal. And a lot can come after this. |
|
@lkstrp I still can't find the time to look into this (even though I intrinsically would like to). So merging after the tests pass would be fine for me as this will unleash so much new potential for the package 🤘 |
1 similar comment
|
@lkstrp I still can't find the time to look into this (even though I intrinsically would like to). So merging after the tests pass would be fine for me as this will unleash so much new potential for the package 🤘 |
|
Ok, to merge from my side when these two TODOs are checked:
|
|
I will also add a couple of improvements and more checks for custom components class since I got some example use cases now. But will also not merge before EDIT: Moving this to another PR, we are too bloated already. |
Ref #936
Changes proposed in this Pull Request
Summary
n.component(),n.generators,n.generators_tand the equivalents in sub-networksComponents Class
ComponentsData->Components->Generators,GenericComponentsetc.ComponentsData: Simple data class containing all stored datastaticanddynamic). Especially regarding the extra dimension that comes with stochastic networks.xarrayorpandas) back to the existing API and keep full backwards compatibility. Sostaticanddynamicare untouched.Components: Abstract base class that contains all logic relevant to all components.Generators,GenericComponentsetc: child classes to be usedself.n = None) or not attached (self.n = n).self.npoints to the attached network,self.attachedis just a boolean indicating the same, andself.n_savecan be used for functionality that requires an attached in any caseself.get_active_assets(), which needs access to investment periods (and even that could be integrated into the components class).SubNetworksn.generators(),sn.generators_i(),sn.components()is not deprecated yet, but points to the same generalized viewComponents Store
n.components.n.componentsis maintained.Dict, but with the possibility of some extra functionality:n.iterate_components()..keys,.values,.itemscould be used)str) is returned. To do anything you would always have to statec = n.components[c_name]andc_namecould also be accessed viac.name.Components Type
components.csv,component_attrs/) are now stored at pypsa module level instead of dynamically during network initialisation.ComponentsTypeInfodata class with all fixed attributes:name,list_name,description,category,defaults(previouslyattrs) andstandard_types(which I moved there).Componentsinstance is associated with a component type (ct) and therefore does not need to store any of the above information.ct,n,staticanddynamic.ToDos
In this PR
API discussion and Docs, moving logic, new features,
xarrayview and so on, I would move to other PRs.