-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add profile plotter and pyswmm integration #165
Conversation
else: | ||
SWMM_ENGINE_PATH = os.path.join(ROOT_DIR, 'lib', 'windows', 'swmm5_22.exe') | ||
# path to the Python executable used to run your version of Python | ||
PYTHON_EXE_PATH = "python"#os.path.join(os.__file__.split("lib/")[0],"bin","python") |
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.
@aerispaha can you give me some advice here? This could break someone's run if they had python pointing to py2.7
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.
Hmm, well we officially dropped support for Python 2.x back in version 0.3.7. Do you think we need to support 2.x to have pyswmm integrated as the model run engine?
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.
@aerispaha nope! If the user is not running inside an environment, this line just calls “python” rather than the version of python you are using to execute.
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.
@bemcdonnell thanks for this submission! I know it's still in draft, but I've got one comment so far: can we remove the swmmio/tests/data/Example1_parallel_loop.out
binary file from the PR? I don't suspect that is needed, right?
@aerispaha, I'm going to make a unit test that uses the OUT file for the HGL plotter so i was hoping to keep that in there. :) |
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.
@bemcdonnell thanks so much for these awesome additions! I've made a few tweaks to the run_models module, and tweaked some syntax here and there. Otherwise, this looks great.
Super excited to have a solid integration between swmmio and pyswmm ❤️
} | ||
|
||
|
||
def test_profile(): |
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.
For some reason, this unit test fails on Apple M1. But it seems fine in the rest of the build matrix. I think we'll need to revisit this when we work on #167. This is okay for this PR.
This PR introduces new plotting utilities and integrates pyswmm as the underlying engine for running models. Additionally, several improvements were made under the hood in the run_models module ❤️
Closes #75
Closes #153
Closes #139
Makes progress on #57 by adding the
report
property to theswmmio.Model.inp
object