Conversation
add legend for arrows
lkstrp
left a comment
lkstrp
left a comment
There was a problem hiding this comment.
Beautiful! Thanks for reviving #912!
I can't quite remember what I had in mind back in the summer, but this looks pretty good.
I have a few things next to the comments:
- A lot of logic could be moved to the component class already, which would reduce boilerplate. I'm not sure if we want to tackle this now or in another PR.
- Is it time to improve the quality of docstrings? We could do this by adding
plot.pyandgeo.pyto the https://github.com/PyPSA/PyPSA/blob/4fc081afac1bfc809533d6c7b2c50dc34652adce/pyproject.toml#L159C1-L159C38. - We need tests. Maybe it's good timing to include the Doctests PR, which would be great for better docs and testing all those simple methods in one go.
- The NetworkPlotter class is still very large. I wonder if we should break it down further into subclasses.
- Both plotting functions have tons of arguments. Not sure if we can improve this somehow, but there must be ways.
| return [i[3:] for i in where if i.startswith("bus") and i not in ["bus0", "bus1"]] | ||
|
|
||
|
|
||
| def bus_carrier_unit(n: Network, bus_carrier: str | Sequence[str] | None) -> str: |
There was a problem hiding this comment.
Could already be moved to the components class. Maybe a new module components/descriptors.py one level down
| @property | ||
| def x(self): | ||
| if self._x is None: | ||
| self.set_layout() |
There was a problem hiding this comment.
set_layout is called in __init__, so self._x will never be None?
Same goes for self._y
pypsa/plot.py
Outdated
| def add_geomap_features(self, geomap=True, geomap_colors=None): | ||
| resolution = "50m" if isinstance(geomap, bool) else geomap |
There was a problem hiding this comment.
I still don't like these arguments/variables that can be bool and a string. Let's get rid of them everywhere.
This could just be:
| def add_geomap_features(self, geomap=True, geomap_colors=None): | |
| resolution = "50m" if isinstance(geomap, bool) else geomap | |
| def add_geomap_features(self, resolution:str="50m", geomap_colors=None): |
and only be called if geomap should be added
| color_geomap="geomap_colors", | ||
| ) | ||
| @wraps(NetworkMapPlotter.static_map) | ||
| def plot( |
There was a problem hiding this comment.
Why get most arguments via the wrap, but define some still here? Is this a clear cut between kwrags for Plotter init and kwargs needed inside plot()?
|
@lkstrp thanks for the review! the playground is open and all your suggestion should go in. I would be very happy to move things to the component structure, this is something I would like to do anyway. |
FabianHofmann
force-pushed
the
plotter-integrated-functions
branch
from
0e59c2c to
cbd5032
Compare
|
Closed via #1156 |
2 participants
Closes #912
This PR builds on top of #912 and adds
NetworkMapPlotter.plot(renamed to .static_map`) function.static_balance_mapwhichflow_area_fraction/bus_area_fractionTODOS:
Changes proposed in this Pull Request
Checklist
doc.doc/release_notes.rstof the upcoming release is included.