-
Notifications
You must be signed in to change notification settings - Fork 135
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 Link Connection and type helper functions #37
Conversation
Updated PR to now cover #18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments :-)
pyswmm/links.py
Outdated
... print c1c2.is_conduit() | ||
>>> True | ||
""" | ||
if self._model.getLinkType(self._linkid) is LinkType.conduit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return self._model.getLinkType(self._linkid) is LinkType.conduit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find!
pyswmm/links.py
Outdated
... print c1c2.is_orifice() | ||
>>> False | ||
""" | ||
if self._model.getLinkType(self._linkid) is LinkType.orifice: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return self._model.getLinkType(self._linkid) is LinkType.orifice
pyswmm/links.py
Outdated
... print c1c2.is_weir() | ||
>>> False | ||
""" | ||
if self._model.getLinkType(self._linkid) is LinkType.weir: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._model.getLinkType(self._linkid) is LinkType.weir
pyswmm/links.py
Outdated
... print c1c2.is_outlet() | ||
>>> False | ||
""" | ||
if self._model.getLinkType(self._linkid) is LinkType.outlet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._model.getLinkType(self._linkid) is LinkType.outlet
pyswmm/links.py
Outdated
return False | ||
|
||
@property | ||
def link_connections(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just connections
maybe? or does it make sense to prefix with link?
@bemcdonnell looking great, I added some comments that are quick changes :-) |
@goanpeca, I updated the helper functions and I added nodes.py. |
# ----------------------------------------------------------------------------- | ||
"""Nodes module for the pythonic interface to SWMM5.""" | ||
|
||
from toolkitapi import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general using the *
on imports is discouraged. its better to be explicit about what gets imported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. In the next milestone I plan to address all of these
pyswmm/nodes.py
Outdated
:rtype: int | ||
|
||
""" | ||
return self._nNodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add
self._nNodes = self._model.getProjectSize(ObjectType.NODE)
directly here?
pyswmm/nodes.py
Outdated
raise StopIteration() | ||
|
||
@property | ||
def nodeid(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node_
inside node object is redundant but id
is a reserved word so maybe nid
otherwise we would need to use id_
, which many libraries do?
Why is there a nodeid in the nodes objetct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way i have set this us is to pass a the ID to instantiate a Node class. So I needed a simple way to get an ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for the iterator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but it feels like a private method then.... _nodeid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice point, I will make it a private method (both nodes.py and links.py)
... 0.0 | ||
|
||
""" | ||
def __init__(self, model, nodeid): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we move forward, passing the model
should be unnecessary cause the Network
object or whatever we decide to call it should be aware of the model already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea. We can raise an issue and talk strategy for the next milestone
def flooding(self): | ||
""" | ||
Get Node Results for Flooding Rate. If Simulation is not running | ||
this method will raise a warning and return 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 0 as opposed to None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because when the simulation has not began, these values are all 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok :-) got it
Covers issue #36
I also updated the docs a little to alleviate link/links/Links/Link confusion... @michaeltryby