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

pipeflow cancellation if all nodes are out-of-service after the connectivity check #564

Closed
wants to merge 7 commits into from

Conversation

SimonRubenDrauz
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d5d25ae) 85.12% compared to head (a15399c) 85.13%.

Files Patch % Lines
pandapipes/pipeflow.py 85.71% 1 Missing ⚠️
setup.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #564   +/-   ##
========================================
  Coverage    85.12%   85.13%           
========================================
  Files           90       90           
  Lines         6179     6181    +2     
========================================
+ Hits          5260     5262    +2     
  Misses         919      919           

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

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.

Great to have this covered. I am just wondering, if it would make more sense to raise an Exception instead of just logging a warning and interrupting the pipeflow. What is the idea behind this? And could we also check net.converged?

@SimonRubenDrauz
Copy link
Collaborator Author

SimonRubenDrauz commented Sep 15, 2023

Right now, what happens is, that it just returns wihtout raising any error. This means that the pipeflow can be conducted even in an empty or non-supplied net. The results would be nans for all nodes, pipes and so on. I prefer this solution as it is consistent if only a certain part of the net is not supplied.
By default calling the pipeflow, net.converged is set False by default. Shall we set it to True or leave it False here? I would add it to the test accordingly!

@SimonRubenDrauz
Copy link
Collaborator Author

@dlohmeier: any new thoughts on this?

dlohmeier
dlohmeier previously approved these changes Nov 17, 2023
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.

All right, I think that's a good solution.

@SimonRubenDrauz
Copy link
Collaborator Author

changed fork branch. Please refer to #579

@SimonRubenDrauz
Copy link
Collaborator Author

changed fork branch. Please refer to #579

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.

3 participants