Skip to content

Conversation

@wraseman
Copy link

@wraseman wraseman commented Jan 7, 2025

Modified CRS data type to match GeoPandas to resolve #239.

Added property for core.inp.symbols to extract rain gage coordinates as a dataframe.

@wraseman wraseman marked this pull request as ready for review January 7, 2025 22:20
@wraseman
Copy link
Author

wraseman commented Jan 7, 2025

@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.

@bemcdonnell
Copy link
Member

@wraseman, nice PR! And great work. @aerispaha, are you still using autopep8 for your auto formatter?

Copy link
Member

@aerispaha aerispaha left a 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]
Copy link
Member

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)

Copy link
Author

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.

links = m.links.dataframe
assert(len(links) == len(G.edges()))

def test_model_to_networkx_crs():
Copy link
Member

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!

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.

Unexpected type for model.crs

3 participants