Added nodes/pipes to excluded to the skel proc #384
Conversation
|
Thanks for adding this feature! Since tanks and reservoirs are not included in skeletonization, I suggest changing the term node to junction in the updated code and documentation. I'll make some notes in the code review. Also, I agree that |
| @@ -82,6 +82,7 @@ This initial set of operations can generate new branch pipes, pipes in series, a | |||
| This cycle repeats until the network can no longer be reduced. | |||
| The user can specify if branch trimming, series pipe merge, and/or parallel pipe merge should be included in the skeletonization operations. | |||
| The user can also specify a maximum number of cycles to include in the process. | |||
| The user can also specify a list of nodes and pipes which should be excluded from skeletonization operations. | |||
There was a problem hiding this comment.
change nodes to junctions
| @@ -43,6 +44,10 @@ def skeletonize(wn, pipe_diameter_threshold, branch_trim=True, series_pipe_merge | |||
| use_epanet: bool, optional | |||
| If True, use the EpanetSimulator to compute headloss in pipes. | |||
| If False, use the WNTRSimulator to compute headloss in pipes. | |||
| pipes_to_exclude: list, optional | |||
| List of pipe names to exclude from skeletonization | |||
| nodes_to_exclude: list, optional | |||
There was a problem hiding this comment.
rename nodes_to_exclude to juncs_to_exclude
| self.pipe_with_controls = list(set(pipe_with_controls)) | ||
| self.pipe_with_controls.extend(pipes_to_exclude) | ||
|
|
There was a problem hiding this comment.
rename self.juncs_with_controls to self.juncs_to_exclude
rename self.pipe_with_controls to self.pipes_to_exclude
|
|
||
| self.assertEqual(skel_wn.num_nodes, wn.num_nodes - 17) | ||
| self.assertEqual(skel_wn.num_links, wn.num_links - 22) | ||
|
|
||
| def test_series_merge_properties(self): |
There was a problem hiding this comment.
This test would be stronger by asserting that pipe 60 and junction 13 are in skel_wn.
There was a problem hiding this comment.
@frank-hulo Will you be able to address the review comments? If not, we can make the minor updates in house.
There was a problem hiding this comment.
@kaklise I could finish it within the next two weeks.
There was a problem hiding this comment.
@frank-hulo We'd like to get this PR merged into main. Let me know if you can address the comments. I can finish it up next week if needed.
| :class:`~wntr.network.model.WaterNetworkModel` | ||
| A water network model object | ||
| tuple | ||
| A tuple of two elements: a dictionary (OrderedDict) of sections, and a list of top comments | ||
|
|
There was a problem hiding this comment.
I don't think the changes in io.py were intended for this PR.
Provide a summary of the proposed changes, describe tests and documentation, and review the acknowledgement below.
#347
Summary
Added nodes and links to exclude in the skelenazation process. It is handled in the same manner as the nodes and links with controls.
Tests and documentation
Test copied from nodes and links with controls, should have the same impact.
Two new inputs added to the initialization of the class, so they can be extended to the nodes -links with controls. They could also be separated, but the if statements and the three separate stages are fairly complex, so I didn't want to add another
andto it.Only the naming could now be improved in the if statements, instead nodes_with_controls -> nodes_to_exclude, which makes it easier to read.
Acknowledgement
By contributing to this software project, I acknowledge that I have reviewed the software quality assurance guidelines and that my contributions are submitted under the Revised BSD License.