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

Features/update naming #43

Merged
merged 10 commits into from
Sep 8, 2020
Merged

Features/update naming #43

merged 10 commits into from
Sep 8, 2020

Conversation

MaGering
Copy link
Contributor

With this PR the naming is adjusted from 'edges' to 'pipes' as suggested in issue #38

@MaGering MaGering requested a review from jnnr August 11, 2020 08:03
@MaGering MaGering self-assigned this Aug 11, 2020
@jnnr jnnr added this to the Release v0.0.1 milestone Aug 25, 2020
dhnx/dhn_from_osm.py Outdated Show resolved Hide resolved
dhnx/graph.py Show resolved Hide resolved
@@ -40,8 +40,8 @@ class InteractiveMap():
"""
def __init__(self, thermal_network):
self.node_data = self.collect_node_data(thermal_network)
Copy link
Member

@jnnr jnnr Sep 7, 2020

Choose a reason for hiding this comment

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

Revert everything to edges, except for
self.edge_data = thermal_network.components.pipes

@@ -48,23 +48,23 @@
# get building data
areas = footprints.area

# get nodes and edges from graph
nodes, edges = ox.save_load.graph_to_gdfs(graph)
Copy link
Member

Choose a reason for hiding this comment

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

It is streets that one gets from osmnx. More abstract: Edges. Stay with edges?

@jnnr
Copy link
Member

jnnr commented Sep 7, 2020

Should add in components.csv's description that pipes have a defined direction.

tests/test_errors.py Outdated Show resolved Hide resolved
@joroeder
Copy link
Member

joroeder commented Sep 7, 2020

it seems that sometimes edges is sometimes more appropriate, e.g. if you do this logical distinction that edges are pipes before everything (like flow direction, capacity) is defined, then, I am wondering about the optimization part. There, in this sense, you have pipes only after the optimization. Before, these are kind of “potential” pipe locations. And in a mashed grid with multiple producers, there can be different load states and flow directions as well. So, I am not sure, if the flow direction should be a must-have for pipes. But, at the moment, I do not have a good idea how to handle. Maybe we need two network attributes: edges and pipes?

@jnnr
Copy link
Member

jnnr commented Sep 7, 2020

it seems that sometimes edges is sometimes more appropriate, e.g. if you do this logical distinction that edges are pipes before everything (like flow direction, capacity) is defined, then, I am wondering about the optimization part. There, in this sense, you have pipes only after the optimization. Before, these are kind of “potential” pipe locations. And in a mashed grid with multiple producers, there can be different load states and flow directions as well. So, I am not sure, if the flow direction should be a must-have for pipes. But, at the moment, I do not have a good idea how to handle. Maybe we need two network attributes: edges and pipes?

The idea of this PR is to rename edges to pipes because that name is more concrete. At some places in functions that handle networkx graphs I decided that it is more natural to keep talking of edges, as the function is more abstract (see the last commits).

What you propose is to use the two terms, edges and pipes, to discriminate beetween potential pipes and existing pipes. I would like to call them one of the two, edges or pipes. As we discussed this in an issue and there were no objections for quite a while, I would like to stay with 'pipes'.

Then we indicate the distinction of presence or not by setting the attribute 'pipe_type', 'capacity' to None and 0 if it does not exist and some value if it exists. See #44.

@joroeder
Copy link
Member

joroeder commented Sep 7, 2020

What you propose is to use the two terms, edges and pipes, to discriminate beetween potential pipes and existing pipes. I would like to call them one of the two, edges or pipes. As we discussed this in an issue and there were no objections for quite a while, I would like to stay with 'pipes'.

Okay, let's do it this way.

@jnnr jnnr self-requested a review September 8, 2020 09:02
Copy link
Member

@jnnr jnnr left a comment

Choose a reason for hiding this comment

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

Thanks for your effort! I reverted the renaming in those cases where the code talks about graphs in a more abstract sense.

@jnnr jnnr merged commit 166a8e5 into dev Sep 8, 2020
@jnnr jnnr deleted the features/update-naming branch September 8, 2020 09:03
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