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

add function for reverse links direction #368

Merged
merged 4 commits into from
Oct 13, 2023
Merged

Conversation

GalPerelman
Copy link
Contributor

This PR adds a function that can reverse links direction
The function preserves the link's vertices

@kaklise
Copy link
Collaborator

kaklise commented Aug 15, 2023

Thanks for contributing this function! Can you provide simple API documentation and a test? I can push an update that will resolve the current test failure.

@kaklise kaklise requested a review from kbonney August 25, 2023 20:25
return wn2


def reverse_link(wn: wntr.network.WaterNetworkModel, link_name: str) -> wntr.network.WaterNetworkModel:
Copy link
Collaborator

Choose a reason for hiding this comment

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

For type comments, please just use the name of the class. In this case WaterNetworkModel. This goes for the function signature and the docstring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend implementing the keyword argument return_copy as is done in wntr.morph.link._split_or_break_pipe. This should default to True to follow the rest of WNTR's API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @GalPerelman. I looked over your changes and left a few small comments to address. Once those are taken care of, we'll be ready to pull in your contribution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@GalPerelman Are you able to update your PR with these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbonney
Sorry for the late response, for some reason I missed your comments
Yes, I will update the PR soon

Copy link
Collaborator

Choose a reason for hiding this comment

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

@GalPerelman, I think I missed the "submit review" button so the notifications only went through recently. Thanks!

@GalPerelman
Copy link
Contributor Author

@kbonney
I updated the PR with the minor changes but now one of the tests is failing
I couldn't figure out why this is happening, can you please help with this?

@kbonney
Copy link
Collaborator

kbonney commented Oct 12, 2023

Thanks for resolving those issues.
These errors show up on occasion. It doesn't have anything to do with your code. @kaklise if we rerun the tests it should clear the failures and we can pull the updates in.

@kaklise kaklise merged commit fb3c07b into USEPA:main Oct 13, 2023
40 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.

3 participants