covid-sim icon indicating copy to clipboard operation
covid-sim copied to clipboard

Need help with figuring out which parameters are optional

Open Feynstein opened this issue 4 years ago • 19 comments

I just took a look at: https://github.com/mrc-ide/covid-sim/blob/master/docs/inputs-and-outputs.md

But I don't see any description of the parameters like which one are mandatory or not...

I especially want to know if those in brackets are.

Halp pls! :-)

Feynstein avatar May 09 '20 23:05 Feynstein

I finally figured it out by myself.

Feynstein avatar May 10 '20 06:05 Feynstein

Finally it would be a good thing to explain which parameters are mandatory... I'm having a hard time figuring this one out from the code only.

Feynstein avatar May 10 '20 18:05 Feynstein

Especially this part because in the documentation the admin file in not enclosed in brackets but in the param loop theres no check for it https://github.com/mrc-ide/covid-sim/blob/01010f52a3a4e2cc8eaa09d144ad21bd1ea88c22/src/CovidSim.cpp#L164

Feynstein avatar May 10 '20 18:05 Feynstein

There are no mention of the kernel strenght in the documentation also https://github.com/mrc-ide/covid-sim/blob/01010f52a3a4e2cc8eaa09d144ad21bd1ea88c22/src/CovidSim.cpp#L184

Feynstein avatar May 10 '20 18:05 Feynstein

It would be good to understand how kernels are used and set. Maybe that's another issue to raise.

insidedctm avatar May 10 '20 19:05 insidedctm

Finally it would be a good thing to explain which parameters are mandatory... I'm having a hard time figuring this one out from the code only.

I attempted to document which command-line parameters were mandatory/optional in #164, at least from the perspective of the main() function. This was merged yesterday, so I would advise taking a second look at the inputs-and-outputs.md file on Master now to see if this documentation helps

Also, I will be making a pull request later this afternoon which refactors the if/else parameter parsing into a much more, I hope, easier-to-read series of switch cases in addition to a '/H' option for a detailed help similar to the inputs-and-outputs.md file.

I will say that even though the main() parameter parsing code seems to only require the /O and /P parameters in addition to the RNG seeds, it appears that there are a lot of hidden requirements in the /P file that are usually provided in the Pre-Param file. I hope to eventually make some headway into documenting that.

ozmorph avatar May 12 '20 14:05 ozmorph

@ozmorph Do you think I can do a pull request of the program_options code I did to replace the very complex parsing at the beginning of the main? When it will be done of course... Since I moved a lot of stuff around in my fork I might reclone it and include only this part. It efficiently clears up all the parsing stuff.

Feynstein avatar May 12 '20 16:05 Feynstein

@Feynstein I actually just submitted my PR #228 that did a lot of work on the main() CLI argument parsing. Can you send me a link to your branch so that I can compare/integrate with it?

ozmorph avatar May 12 '20 16:05 ozmorph

@ozmorph Of course! https://github.com/Feynstein/covid-sim https://github.com/Feynstein/covid-sim/blob/master/CovidSim.cpp

What you want to look at is my main... Its been changed a lot and I upgraded to C++17 in order to use the std::any but I think that you could easily copy-paste the program options part and grow from it to what is necessary. I would like to be quoted as a collaborator if you do though :-) This is a good thing for a scientific dev like me :P

Feynstein avatar May 12 '20 16:05 Feynstein

Oh and in my CMake Lists I added an automatic out-of-source -git-branch-related-debug/release build, so that when you get in there you only have to do "cmake ."

Feynstein avatar May 12 '20 16:05 Feynstein

And I already sent an email from my university account to @weshinsley in order to see if I could be an official scientific contributor...

Feynstein avatar May 12 '20 16:05 Feynstein

Thanks, I will take a look at it. If I am asked to make any changes in #228, then I will consult your code to see if there is a middle ground for both implementations and adjust accordingly.

And of course, I will give you acknowledgement of collaborating if I do integrate your changes.

ozmorph avatar May 12 '20 17:05 ozmorph

@ozmorph ohhh there something I forgot when I looked at #228. There would be a very easy way to verify if the files exist using boost::filesystem or std::filesystem (I think this one is in it). I did not do it now, but it would be easy to add to the checks done on the arguments themselves.

In order to use program_options, which I dont know if you know, you simply add your arguments like: --preParamF=/some/file

and for the multiple files you reuse the same call: --interventionsF=/some/file1 --interventionsF=/some/file2

Feynstein avatar May 12 '20 17:05 Feynstein

@Feynstein @ozmorph Yohan no you can't do that for #228. The build is C++11, std::filesystem is C++17. git submodules seem to be ignored by the CI system so you won't get boost in either.

zebmason avatar May 12 '20 17:05 zebmason

@zebmason Well... It seems to me that this could be added as a pressing issue in order to be able to make any significant improvement on the original code... boost is used almost everywhere in the scientific code I use... And probably in most other code that I don't know about.

Feynstein avatar May 12 '20 18:05 Feynstein

@Feynstein Yohan there is a backport of std::filesystem that I've used in the past and I have a approved commit for C++14 (#207) and may get submodules sorted due to #213

zebmason avatar May 12 '20 18:05 zebmason

@zebmason I already filed an issue #229 about this because in itself is a very pressing issue imo... and also since its not really related to this one (definitions of parameters) you can comment there for a better issue modularity. What you just said basically.

Feynstein avatar May 12 '20 18:05 Feynstein

Thanks for your time on this. Many parameters have defaults, which isn’t quite the same as being optional. We will try to generate some “minimal” parameter files in the coming days and weeks though. It is possible to run the code as a simple non-spatial stochastic SEIR model with no households or places with a tiny number of parameters. But we will also try to generate some more functional minimalist parameter files.

NeilFerguson avatar May 21 '20 20:05 NeilFerguson

@NeilFerguson Thanks for your answer, really appreciated. I will try to continue my work on this. I've started a pretty big refactoring of the code that I might present as a parallel, object oriented version of this. But for now I can't because I'm sick and my day job reopened. I don't know if I will be able to completely refactor this thing but I think that I brought a few fresh ideas that might help the future scientific development of this simulation. In the best of worlds everything should be modular so that new grad students or scientists that are unfamiliar with C++ can create his own sub-library and call it from the relevant place in the code, without risking to damage it. That's what I will try to improve next. Thanks and keep up the good work!

Feynstein avatar May 22 '20 17:05 Feynstein