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

Hotstart save (aka "Bread Crumbs") #252

Closed
wants to merge 18 commits into from

Conversation

bemcdonnell
Copy link
Member

@bemcdonnell bemcdonnell commented Jul 30, 2019

Continuing the great contribution from @jsadler2.

Builds on top of #179

Closes #253

@codecov-io
Copy link

codecov-io commented Jul 30, 2019

Codecov Report

Merging #252 into develop will increase coverage by 0.46%.
The diff coverage is 81.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #252      +/-   ##
===========================================
+ Coverage    62.55%   63.02%   +0.46%     
===========================================
  Files           53       53              
  Lines        16672    16694      +22     
===========================================
+ Hits         10430    10521      +91     
+ Misses        6242     6173      -69
Impacted Files Coverage Δ
src/swmm5.c 85.11% <100%> (ø) ⬆️
src/hotstart.c 75% <80%> (+22.39%) ⬆️
src/toolkitAPI.c 66.34% <84.61%> (+1.5%) ⬆️
src/massbal.c 79.02% <0%> (+0.21%) ⬆️
src/infil.c 61.51% <0%> (+3.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a838227...18fe690. Read the comment docs.

@jsadler2
Copy link

@bemcdonnell , thanks for pushing this forward.

@bemcdonnell bemcdonnell changed the title Dev hotstart Hotstart save (aka "Bread Crumbs") Jul 30, 2019
@bemcdonnell bemcdonnell requested review from cbuahin and jennwuu July 30, 2019 23:07
@bemcdonnell bemcdonnell added this to the v5.3.0.dev0 milestone Jul 30, 2019
@bemcdonnell bemcdonnell requested a review from LRossman July 30, 2019 23:24
@bemcdonnell
Copy link
Member Author

@LRossman, I did just a tiny bit of refactoring for saving out to an HSF. Mainly passing the file structure pointer as an argument. I can't imagine this will be a deal breaker - but it seems to make a little more sense this way considering we are opening up this as a new API feature.

@bemcdonnell
Copy link
Member Author

@LRossman, I'm also leaning toward changing the function names a little too. hotstart_open has more of hotstart_set_initial_state behavior. And hotstart_close has more of hotstart_save_state. The proposed function names could be simpler, yes, but the idea is that they are a little more descriptive/less ambiguous. Thoughts?

@LRossman
Copy link

@bemcdonnell as I'm reviewing this PR I noticed that the hotstart.c file being modified seems to come from v.5.1.012 (or maybe 5.1.011) and not 5.1.013 since the latter has a code section starting at line 77 removed whereas your file just has the section commented out. Now this may be moot, since there are no comments in the file marking any changes for v.5.1.013, but it has me concerned that the project may not be using all of the files from the latest EPA release (5.1.013) as its base.

@LRossman
Copy link

LRossman commented Jul 31, 2019

I don't like creating 3 new functions for saving a hotstart file via the swmm_saveHotstart function. Here's how I would have done it without needing to change anything in the original SWMM code:

int DLLEXPORT swmm_saveHotstart(char *hsfile)
{
// Sanity checks go here
    else
    {
        TFile tmpHotstart1 = Fhotstart1;
        TFile tmpHotstart2 = Fhotstart2;
        Fhotstart1.mode = NO_FILE;
        Fhotstart2.mode = SAVE_FILE;
        sstrncpy(Fhotstart2.name, hsfile, MAXFNAME); 
        if (hotstart_open()) hotstart_close();
        Fhotstart2 = tmpHotstart2;
        Fhotstart1 = tmpHotstart1;
    }
    return error_getCode(error_code_index);
}

Copy link

@LRossman LRossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my suggestion for implementing swmm_saveHotstart without having to change any of the existing SWMM code.

@cbuahin
Copy link

cbuahin commented Jul 31, 2019

@bemcdonnell I am trying to think of reasons why the implementation suggested by @LRossman will not be ideal but can't think of any major ones save for I am generally apprehensive about using global variables and repurposing one like this makes me worry even more. However, it is less intrusive and accomplishes the same task. What do you think ?

@bemcdonnell
Copy link
Member Author

@LRossman, I like the simplicity and elegance of your suggested implementation. I wonder if perhaps we could consider a blended solution that could perhaps be more congruent with longer-term planning for this project (echoing @calebbuahin). I could be being way too presumptuous here but what if we think about our end-goal as a place where we encapsulate all of the globals and separate concerns better? By doing so, we can write code that is easily unit-testable. I think opening up an issue to talk about more general roadmapping philosophy would be worth all of our time! So you be the judge in this thread - Am I being presumptuous or not with my proposed end-goal? :-)

What I like about @jsadler2's and my proposed implementation is that we are passing file pointers into a function as an argument instead of telling this function to use a global. To me, this makes the code a lot more readable. What I don't like about the proposed implementation is exactly what you are saying @LRossman - we are explicitly calling saveRouting and saveRunoff in toolkitAPI.c. Directly talking to those two functions outside of hotstart.c is NOT elegant. I think swmm_saveHotstart(char *flname) should call a function like hotstart_close(TFile flptr). One thing I really do not like is using temporary variables to hold and store state. It doesn't feel as clean to me - but this is totally a personal style thing. Passing file handles around is a nice step toward an encapsulation. Also it will make it easier to unit test.

What are your thoughts?

Oh and a quick response to what you found:

@bemcdonnell as I'm reviewing this PR I noticed that the hotstart.c file being modified seems to come from v.5.1.012 (or maybe 5.1.011) and not 5.1.013

This came from a dev version of 5.1.013. One of the last things that was done was comments were stripped away. Looks like I introduced the comment back it when i resolved the merge conflicts into Jeff's code. I promise this is the only thing that I brought back in ;-)

@LRossman
Copy link

@bemcdonnell I thought that the focus of this project was to supply SWMM with a comprehensive API, only making changes to the core engine code when it was absolutely necessary, thus making it easier to keep in synch with the official EPA version of the code. Although I'm sure there are lots of places where refactoring might be called for, my response would be "if it ain't broke don't fix it".

That being said, I would endorse someone or group forking the code to another project (not just a branch) and re-architecting it from the ground up (not just applying piecemeal refactoring to the current code). It's been almost 15 years since SWMM 5 was first released so maybe the software development cycle has run its course.

One source of ideas for what the re-design might look like can be found in a white paper I wrote for the re-design of EPANET. EPANET and SWMM have a lot in common from a structural point of view so the ideas presented there may be relevant.

@bemcdonnell
Copy link
Member Author

@LRossman, I've elevated your last comment to a new Issue in #254. I'm writing a response to it now!

@bemcdonnell bemcdonnell removed this from the v5.3.0.dev0 milestone May 4, 2022
@bemcdonnell bemcdonnell removed their assignment May 4, 2022
@bemcdonnell
Copy link
Member Author

@jackieff has been newly assigned to this since she sounded really excited to do it! Thanks @jackieff !!!

@bemcdonnell
Copy link
Member Author

#393 has replaced this PR.

@bemcdonnell bemcdonnell deleted the dev_hotstart branch June 11, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add save_hotstart file to API ("Breadcrumbs" feature")
6 participants