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

Modified API for initial conditions #392

Closed
wants to merge 1 commit into from
Closed

Conversation

bemcdonnell
Copy link
Member

First part in addressing pyswmm-445

@karosc, i learned a new command to type before making a commit to swmm... if you have a million white space changes, this command saves you git diff -w | git apply --cached --ignore-whitespace

This will solve the initial conditions problem for network levels. The "settings" are solved just by organizing the sim.start()

@bemcdonnell bemcdonnell requested a review from karosc June 9, 2023 21:46
@bemcdonnell bemcdonnell self-assigned this Jun 9, 2023
@karosc
Copy link
Member

karosc commented Jun 10, 2023

@bemcdonnell, Node.initDepth is only used for setting the initial node depth in node_initState(), which is called in project_init(), which is called in swmm_start().

From my point of view, allowing the API to set the initial depth after the simulation start would have no effect on the initial conditions since oldDepth and newDepth will have already been set

Node[j].oldDepth = Node[j].initDepth;

Looking at the pyswmm api, I'd be in favor of deprecating the initial_conditions property in favor of an additional callback: Simulation.after_start. This would give users a clear path forward when they encounter errors when trying to set initial conditions at the wrong point in the simulation flow. Some props can only be set prior to simulation start (e.g. initDepth), while some must be set after start (e.g. target_setting). Ideally pyswmm throws a reasonably descriptive error that can point users to the simulation state (i.e. opened, started, closed) in which setters are valid.

@bemcdonnell
Copy link
Member Author

@karosc, I can't help but agree with your idea. the "after_start" in pyswmm is a great idea. I think when i wrote those callback hooks, "before_step" was to accomplish the same thing. And you're correct, if adding an "after_start" will allow the target_settings to be made. I will close this PR and just make the changes in PySWMM. Thanks for the review!

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.

None yet

2 participants