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

Need help with figuring out which parameters are optional #202

Open
Feynstein opened this issue May 9, 2020 · 19 comments
Open

Need help with figuring out which parameters are optional #202

Feynstein opened this issue May 9, 2020 · 19 comments

Comments

@Feynstein
Copy link

Feynstein commented May 9, 2020

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
Copy link
Author

I finally figured it out by myself.

@Feynstein Feynstein reopened this May 10, 2020
@Feynstein
Copy link
Author

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
Copy link
Author

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

else if (argv[i][1] == 'A' && argv[i][2] == ':')

@Feynstein
Copy link
Author

There are no mention of the kernel strenght in the documentation also

else if (argv[i][1] == 'K' && argv[i][2] == 'P' && argv[i][3] == ':') //added Kernel Power and Offset scaling so that it can easily be altered from the command line in order to vary the kernel quickly: ggilani - 15/10/14

@insidedctm
Copy link
Contributor

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

@ozmorph
Copy link
Contributor

ozmorph commented May 12, 2020

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.

@Feynstein
Copy link
Author

@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.

@ozmorph
Copy link
Contributor

ozmorph commented May 12, 2020

@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?

@Feynstein
Copy link
Author

@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
Copy link
Author

Feynstein commented May 12, 2020

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
Copy link
Author

Feynstein commented May 12, 2020

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

@ozmorph
Copy link
Contributor

ozmorph commented May 12, 2020

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.

@Feynstein
Copy link
Author

Feynstein commented May 12, 2020

@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

@zebmason
Copy link
Contributor

@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.

@Feynstein
Copy link
Author

Feynstein commented May 12, 2020

@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.

@zebmason
Copy link
Contributor

@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

@Feynstein
Copy link
Author

Feynstein commented May 12, 2020

@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.

@NeilFerguson
Copy link
Collaborator

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.

@Feynstein
Copy link
Author

@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!

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

No branches or pull requests

5 participants