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

Added unsupplied_junctions and elements_on_path to pandapipes #592

Merged
merged 13 commits into from
Mar 26, 2024

Conversation

mfranz13
Copy link
Contributor

Added unsupplied_junctions and elements_on_path with tests to pandapipes.
Added the convenience functions get_all_branch_component_table_names and get_all_branch_component_models to prevent hardcoding the branch element table names like in pandapower.

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: Patch coverage is 71.71717% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 90.49%. Comparing base (613fe60) to head (ac6b9b2).

Files Patch % Lines
src/pandapipes/plotting/pipeflow_results.py 9.67% 28 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #592      +/-   ##
===========================================
- Coverage    90.62%   90.49%   -0.14%     
===========================================
  Files          138      139       +1     
  Lines        10457    10556      +99     
===========================================
+ Hits          9477     9553      +76     
- Misses         980     1003      +23     

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


**element** (string, "l") - element type of type BranchComponent

**multi** (boolean, True) - True: Applied on a NetworkX MultiGraph
Copy link
Collaborator

@dlohmeier dlohmeier Feb 27, 2024

Choose a reason for hiding this comment

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

I guess "multi" is not needed in the docs?

"""
table_names = get_all_branch_component_table_names()
if element not in table_names:
raise ValueError("Invalid element type %s" % element)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I understand, this check is not required to prevent errors. Could we make it optional via another default argument?

"""
def get_all_subclasses(cls):
all_subclasses = list()
for subclass in cls.__subclasses__():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does __subclasses__ search for appearances in the whole python path? If not, we might need to find a different implementation that checks the appearing classes inside the net...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, subclasses includes all child classes and is not dependent from net

Copy link
Collaborator

@dlohmeier dlohmeier left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the great additional features! I like this implementation. just added a few small comments.

@mfranz13
Copy link
Contributor Author

@dlohmeier thx for your suggestions, i implemented the changes :)

mfranz13 and others added 2 commits March 18, 2024 14:08
new function calc_distance_to_junctions
new function plot_pressure_profile
@mfranz13
Copy link
Contributor Author

@dlohmeier I added the functions for plot_pressure_profile to this PR aswell

return [mg.get_edge_data(b1, b2)["key"][1] for b1, b2 in zip(path, path[1:])
if mg.get_edge_data(b1, b2)["key"][0] == element]


if __name__ == '__main__':
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not really necessary, the file should just be seen as a module from where to load functions. Can we just delete the "main" part?

Copy link
Collaborator

@dlohmeier dlohmeier left a comment

Choose a reason for hiding this comment

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

In general, this looks good to me. I am just wondering if it is possible to make the pressure plot vectorized.

Copy link
Collaborator

@dlohmeier dlohmeier left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot for this great added feature!

@dlohmeier dlohmeier merged commit 2883f24 into e2nIEE:develop Mar 26, 2024
21 of 23 checks passed
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.

2 participants