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") #179

Closed
wants to merge 9 commits into from

Conversation

jsadler2
Copy link

@jsadler2 jsadler2 commented May 4, 2018

Here are modifications to enable saving hotstart files during a simulation. This allows users to leave "bread crumbs" during a simulation so they can restart a simulation from a custom point.

@jsadler2
Copy link
Author

jsadler2 commented May 4, 2018

@bemcdonnell, this is the functionality that we discussed a few weeks ago.

@bemcdonnell
Copy link
Member

@jsadler2, If you check the appveyor and travis logs, it will tell you what's going on with your compilation. Just for point of reference, what compiler are you building with on you local machine? And did you run the regression tests?

@bemcdonnell
Copy link
Member

@jsadler2, my first suggesting is to rebase your changes off the most recent develop branch.

@jsadler2
Copy link
Author

jsadler2 commented May 7, 2018

Thanks, @bemcdonnell. I'll rebase and go from there. I am using the linux gcc compiler. I have not run the regression tests locally. How is that done?

@bemcdonnell
Copy link
Member

@jsadler2, https://github.com/OpenWaterAnalytics/Stormwater-Management-Model/blob/develop/.github/REGRESSION_TESTING.md

@bemcdonnell
Copy link
Member

@jsadler2, i'm going to check to see if I can push past the build issue. I overcame some very strange behavior like this with the VC2010 compiler on my own machine.

@jsadler2
Copy link
Author

@bemcdonnell okay. Thanks. I'll keep working on it too.

@bemcdonnell
Copy link
Member

@jsadler2, could you please sign the contributors license agreement (see CLAHUB, above). Also, we still need to diagnose what is going on with the visual studio build.

@bemcdonnell bemcdonnell changed the title Hotstart save Hotstart save (aka "Bread Crumbs") Jun 14, 2018
Copy link
Member

@bemcdonnell bemcdonnell left a comment

Choose a reason for hiding this comment

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

@jsadler2, Nice work on this PR. I have a couple of questions/thoughts.

As I commented before, we have to figure out what is going on with Visual Studio build. Also, we have a fully running set of unit tests now. Please add the unit tests for this new functionality. You can generate your own test_breadCrumbs.cpp if you want to. There are two genres of unit tests - 1. error checking asserting that you get the correct errors and 2. mechanical tests that show whether it is working.

I'm wondering if the original built-in functionality of the HSF still works as you are saving new HSFs. Can you confirm this with a unit test? I may circle back with some other comments as soon as I get a few minutes to fetch these changes and build it locally.

@brief Save hotstart file during simulation
@param hsfile The file name of the hotstart file that user would like to save
*/
void DLLEXPORT save_hotstart(char *hsfile);
Copy link
Member

Choose a reason for hiding this comment

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

Sticking with the convention, please pre-pend the exportable functions with swmm_*

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I will do that.

@@ -1524,6 +1526,40 @@ int DLLEXPORT swmm_setGagePrecip(int index, double total_precip)
return(errcode);
}

void DLLEXPORT save_hotstart(char *hsfile)
Copy link
Member

Choose a reason for hiding this comment

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

@jsadler2, 4 spaces instead of tabs.

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha.

@jsadler2
Copy link
Author

@bemcdonnell roger that about the unit tests.

@bemcdonnell
Copy link
Member

@jsadler2, could you provide an update on the status of this PR? If you decide not to continue to develop it, we can merge it into a feature branch and then we can merge it in down the road once it the tests are built out. Just let me know what is best for you!

@jsadler2
Copy link
Author

Hey @bemcdonnell, thanks for checking in on this. I've had to put this on the back-burner. I would like to continue developing it though. Is it okay for you guys to have the unit tests and the changes you requested in three weeks (by 11/13)?

@jsadler2
Copy link
Author

@bemcdonnell a few weeks later and I still haven't had a chance to look at this PR more. At this point, unfortunately, I don't think I can really put much time into this.

@dickinsonre
Copy link

This is an interesting idea, is the general concept to have a date in the hot start file so that a new run can be started at any time?

@bemcdonnell
Copy link
Member

@jsadler2, do you mind if I takeover this pull request?

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.

3 participants