Conversation
FabianHofmann
left a comment
FabianHofmann
left a comment
There was a problem hiding this comment.
this is a very good direction! Just looked through it and I think we are on a good track. some minor comments below. I guess the plan is to use the NetworkPlotter (which we could also name NetworkMapPlotter if that is specifying its purpose) for n.iplot as well?
pypsa/utils.py
Outdated
| def rename_kwargs(func_name: str, kwargs: dict[str, Any], aliases: dict[str, str]): | ||
| """ | ||
| Helper function for deprecating function arguments. | ||
|
|
||
| Based on solution from [here](https://stackoverflow.com/questions/49802412). | ||
|
|
||
| Parameters | ||
| ---------- | ||
| func_name : str | ||
| The name of the function. | ||
| kwargs : dict | ||
| The keyword arguments of the function. | ||
| aliases : dict | ||
| A mapping of old argument names to new argument names. | ||
|
|
||
| """ | ||
|
|
||
| for alias, new in aliases.items(): | ||
| if alias in kwargs: | ||
| if new in kwargs: | ||
| raise TypeError( | ||
| f"{func_name} received both {alias} and {new} as arguments!" | ||
| f" {alias} is deprecated, use {new} instead." | ||
| ) | ||
| warnings.warn( | ||
| message=( | ||
| f"`{alias}` is deprecated as an argument to `{func_name}`; use" | ||
| f" `{new}` instead." | ||
| ), | ||
| category=DeprecationWarning, | ||
| stacklevel=3, | ||
| ) | ||
| kwargs[new] = kwargs.pop(alias) |
| self.ax = None # Initialized in init_axis | ||
| self.area_factor = 1 # Initialized in init_axis | ||
|
|
||
| def init_layout(self, layouter: nx.drawing.layout = None): |
There was a problem hiding this comment.
perhaps rather name it set_... instead of init_... to be close to the matplotlib naming
| if not isinstance(projection, cartopy.crs.Projection): | ||
| self.boundaries = boundaries | ||
|
|
||
| def _add_geomap_features(self, geomap=True, geomap_colors=None): |
There was a problem hiding this comment.
I would keep that as a non-hidden function, so that the use can add the geomap layer after initializing a plot
| def init_axis(self, ax, projection, geomap, geomap_colors, title): | ||
| # Set up plot (either cartopy or matplotlib) | ||
|
|
||
| transform = get_projection_from_crs(self.n.srid) |
There was a problem hiding this comment.
rather use self.n.crs here
| for branch in branches.values(): | ||
| plotter.ax.add_collection(branch) | ||
|
|
||
| # Plot flows |
There was a problem hiding this comment.
this still feels a bit weird that we are defining all branch components separately to then put them together into the get_branch_collections function where we iterate over them again. I would vote for making get_branch_collection process one branch component at a time
Fine with renaming, but maybe in order to not swap meaning of True/False consider
This should make sense. Geo-features are then automatically activated if
I think arguments for branch components should remain separately configurable. At some point we had an aggregated argument for all branch components where one had to supply a dictionary with keys for "Line", "Link", etc. or a pd.MultiIndex with the first level referring to the compontent. We preferred the flat format with duplication.
Yes, should not be necessary, especially if no elements are created when link_widths=0 (skipped by width threshold).
Would not prioritize the collections. I think few people are accessing this afterwards, but dict seems to be a good idea. |
|
Closed via #1156 |
3 participants
Here is a general overhaul of the
plots.pymodule:plots.pyn.plot()withNetworkPlotterclassflowtoline_flow,link_flow,transformer_flow. See issue belowbus_norm,line_norm,link_norm,transformer_normtox_cmap_normcolor_geomaptogeomap_colorsflowintoline_flow,link_flow,transformer_flowto avoid multiindex #302requestslibrary is not needed anymoregeo.pyplot.pyhereutils.pywith changed kwargs deprecation decoratorDeprecationWarninginstead of logger warningsTodos:
n.iplot()with new structureDiscuss:
geomaptodisable_geomapand add ageomap_resargument, instead of mixing typesgeomapand include it intoprojectionbranch_componentscould be dropped and controlled via attribute argumentsn.plot()Let me know what you think @FabianHofmann @fneum