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

Feature/additional patches #2114

Open
wants to merge 51 commits into
base: develop
Choose a base branch
from

Conversation

hzraja
Copy link
Contributor

@hzraja hzraja commented Aug 16, 2023

  • Added patch for the Wind turbine
  • Added patch for PV
  • static alignment for generic patches based on the different patch type at each bus (3 patches -> 120°, 4 patches -> 90°)
  • Fix visual Bugs
  • Fixed Pandapipes compatibility issue
  • Add changes to \pandapower\CHANGELOG.rs

hzraja and others added 30 commits March 21, 2023 15:21
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.66%. Comparing base (873b90e) to head (9a55153).
Report is 49 commits behind head on develop.

❗ Current head 9a55153 differs from pull request most recent head 8dc50bd. Consider uploading reports for the commit 8dc50bd to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2114      +/-   ##
===========================================
- Coverage    79.84%   79.66%   -0.18%     
===========================================
  Files          255      255              
  Lines        27971    27822     -149     
===========================================
- Hits         22333    22164     -169     
- Misses        5638     5658      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -995,18 +998,17 @@ def create_load_collection(net, loads=None, size=1., infofunc=None, orientation=
"""
loads = get_index_array(loads, net.load.index)
infos = [infofunc(i) for i in range(len(loads))] if infofunc is not None else []
node_coords = net.bus_geodata.loc[net.load.loc[loads, "bus"].values, ["x", "y"]].values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we allowed to remove this statement?

try:
angle = unique_angles[i]["sgen"]["WT"] + (angles[i] or 0)
except IndexError:
continue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would an index error mean? In what case could it occur? I wouldn't necessarily "hide" an exception that is thrown, because it could indicate a wrong implementation somewhere, except we are sure that it is more useful to just not plot some node elements when the angles or unique_angles are not given correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were some edge cases where angles were not in sync with node_coords so used exceptions, This is removed in the upcoming commit

lines.append((perp_foot1, line_end1))
lines.append((perp_foot2, line_end2))
else:
continue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any option for plotting without "unique_angles"? To me it looks like there isn't. I also think that defining the type of patch you want to draw via a variable called "unique_angles" is not very transparent. I would prefer some sort of "type" lookup that defines the type of patch used for each sgen and its angle. If it is not given for any of the indices, that sgen has the default patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added option to work without unique_angles in the upcoming commit.

gen_count = 0
load_count = 0
if plot_sgens and len(net.sgen):
sgen_types_counts = net.sgen[net.sgen.bus == i].type.value_counts()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this is a lot of hard coded and rather implicit indication of the patch types drawn for a simple_plot. I am not too sure how users would react to that. I could agree on this, because it is "just" simple_plot, but in the dedicated plotting functions, we need room for adaptions by the user.

@dlohmeier
Copy link
Collaborator

I like the idea of having new patches for wind turbines and PV plants, but I doubt that the implementation does exactly what it is supposed to do. A few aspects that I would like to clarify:

  • Are there any tests for this to get some information on what is plotted in different cases? I don't mean that it is necessary to draw plots and have some tests check what is on the drawing pane, but at least try to get some more output from the plotting functions to check for different corner cases...
  • Could you provide a small example of what is drawn in what case in the chat? I would prefer not to have to create my own plot from this to know what we are discussing about.
  • What do you think the user will do with this option of choosing between different patch types? Do you think that anyone would like to create different patches in the same collection? If not, then I would suggest to create new functions with the different patch types that could be called separately. So, say 99.9% of users using this function will have to go through the pain to call create_sgen_collection with a subset of sgens and a complex syntax for the unique_angles keyword, while only 0.1% of the users benefit from calling this function just once for all different types of sgens. On the other hand, those 99.9% could just call create_wind_turbine_collection on the subset without even having to pass any unique_angles. That would make it a lot easier to use and we could even speed up the function, as we don't have to do complex operations on the unique_angles argument. In that case it might also be easier to add some kwarg to the simple_plot function like "draw_sgens_by_type" that just differentiates between the old and the new implementation.
  • Did you check on the performance of this implementation? It appears to me that there are lots of loops with lots of complex calculations inside, which would slow down the collection creation tremendously in case of large data.

@hzraja
Copy link
Contributor Author

hzraja commented Dec 8, 2023

@dlohmeier @jwiemer112

The new patches implementation can be utilized with simple_plot as well as create collections, in both cases user has the capability to use or not use the feature (in which case the old implementation kicks in)

Attached herewith are the examples of some user created scenarios with and without the new implementation

pp.plotting.simple_plot(net, plot_sgens=True, plot_gens=True, plot_loads=True, orientation=0, draw_sgens_by_type=False)
Simple_plot_default
The patches here overlap at one default position regardless of how many sgens are created

pp.plotting.simple_plot(net, plot_sgens=True, plot_gens=True, plot_loads=True, orientation=0, draw_sgens_by_type=True)
Simple_plot_new
If user want properly seperated patches (dynamic based on no of sgens), simply setting draw_sgens_by_type = True would result in the attached output. WT and PV have new patches if user sets the sgen type to them, whereas default sgen patch for wye type sgens.

*Create collections (simple_plot not used)

Old implementation:
ax=pp.plotting.create_sgen_collection(net, size=0.1, infofunc=None, orientation=0., picker=False)
collections_oldJPG
Again, the patches here overlap regardless of their count

New implementation (only unique_angles, no new patches)
angles = calculate_unique_angles(net)

ax=pp.plotting.create_sgen_collection(net, size=0.1, infofunc=None, orientation=0., picker=False, unique_angles=angles, draw_sgens_by_type=False)
Collections_new_sgen-patches-is-false
Default patches albeit properly seperated

New implementation (unique_angles, new patches)
angles = calculate_unique_angles(net)

ax=pp.plotting.create_sgen_collection(net, size=0.1, infofunc=None, orientation=0., picker=False, unique_angles=angles, draw_sgens_by_type=True)
Collections_new_sgen-patches-is-true
Properly seperated and new patches for WT and PV if user has selected the type to be of them.

@vogt31337
Copy link
Contributor

Looks nice, but please resolve the conflicts and add an entry to the changelog. @hzraja @jwiemer112

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

Successfully merging this pull request may close these issues.

5 participants