-
Notifications
You must be signed in to change notification settings - Fork 44
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
Modify CRS data type and add [SYMBOLS] for rain gage coordinates #242
base: master
Are you sure you want to change the base?
Conversation
@aerispaha and @bemcdonnell, this is my very first pull request and my first time implementing unit tests, so this code will need a critical eye. |
@wraseman, nice PR! And great work. @aerispaha, are you still using autopep8 for your auto formatter? |
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.
@wraseman I like these contributions! Everything looks good to me.
However, there is an unrelated test, test_inp_sections
, that is failing here as well as in the master branch. Before we merge I'd like to figure out what is going on with that.
""" | ||
Get/set symbols section of INP file. | ||
|
||
Section: [SYMBOLS] |
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.
Love this docstring! It probably will be a good idea to mimic something like this in all the other sections (something we can tackle later)
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'm glad you like it! I blended the definitions in page 341 of the SWMM manual with the field naming convention in inp_sections.yml. I tried to use the exact language from SWMM where I could.
@@ -51,6 +52,12 @@ def test_model_to_networkx(): | |||
links = m.links.dataframe | |||
assert(len(links) == len(G.edges())) | |||
|
|||
def test_model_to_networkx_crs(): |
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.
This is a good test!
Modified CRS data type to match GeoPandas to resolve #239.
Added property for core.inp.symbols to extract rain gage coordinates as a dataframe.