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

Modify CRS data type and add [SYMBOLS] for rain gage coordinates #242

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

@@ -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():
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