-
Notifications
You must be signed in to change notification settings - Fork 350
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
Command line runs #725
Command line runs #725
Conversation
…d line. Add a corresponding yarn target.
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/covid19-scenarios/covid19-scenarios/r6ukndfu4 |
Code Climate has analyzed commit 0bc426b and detected 0 issues on this pull request. View more on Code Climate. |
@bettbra Hi Bradley, However, before merging, I need to ask the folks at GitHub/Microsoft who originally contributed it in #689. @adityasharad Could you please review the changes and see if it is okay for your use-case? I'll be happy to merge then. @bettbra Could you please tell us more about your project, so that we could take this information into account when planning the improvements of the algorithm? Any additional improvements are welcome, including any of the items listed in #663 |
Hi Ivan, Thanks for your reply. Right, I should have checked to try to infer if another team was also using the CLI. I suspect the changes here won't work for them (e.g., @adityasharad) as they already have dependencies on the existing form, but maybe we can coordinate. For us it's convenient to be able to control all parameters---scenario, age, severity---at the command line, meaning not only the first set. So really the change here is focused on adding the last two (age, severity) and using yargs for argument parsing purely for convenience. I also see a bunch of eslint warnings that I'll fix; forgive me, didn't realize those were getting run. I'll eliminate all the warnings to adhere to the project conventions. As for what we're using the project for, we're trying to vary R_0 using mobility data, very similar in spirit to #715. We execute large numbers of runs as we assess sensitivities to various input parameters, doing this on the command line in a large gridded compute environment. The UI is fantastic and we use it for quick sanity checks, but our heavy lifting is done via the CLI. Apologies for the long reply. If the community would favor making the CLI a supported interface we'd be enthusiastic supporters and commit to helping maintain it. Thanks again @ivan-aksamentov. I'll await additional overall feedback and at least fix those warnings as mentioned. --Brad |
Yes. That is probably the most useful interface for the community. Feel free to propose the improvements to the interface that are useful for you. For example we could make every parameter to be exposed from both files and command line, and command line would take precedence. This way one could have a base config and vary a given parameter. We may also read json from stdin, so that users could pipe the autogenerated configs through the shell or exec.
Note that the current eslint config is mostly tailored for the React app, so some of these warning may not necessarily make much sense for a Node script. In this case, feel free to either (1) add an eslint-ignore comment for the file or (2) add an item in the
Wow, that sounds cool! Do you have a link?
That would be fantastic! Feel free to pick any if the items in #663 and add yours too. We are looking forward to any feedback and contributions. I guess currently you have to pull the project from GitHub in order to use it, maybe as a submodule or similar. Would it be useful if we create and NPM package with a CLI and programmatic Node.js interface? We could automate the build and publishing as well. One advantage is more convenient versioning and reduction of breakage, because we are currently not tracking how our changes affect the CLI at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely! I think this will work nicely with the way we're currently using the model, and is certainly better than my initial version. Thanks @ivan-aksamentov for keeping me in the loop.
Minor suggestions about typings, otherwise looks good. I defer to Ivan on the eslint warnings - some of them look easy to fix but I have no strong feelings about suppressing vs fixing them.
@bettbra we (on the GitHub side) would love to hear more about your work on running and evaluating different models.
*/ | ||
function getSeverity(inputFilename: string | undefined) { | ||
if (inputFilename) { | ||
const data = readJsonFromFile<any>(inputFilename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can type this as SeverityDistributionData
instead of any
. That type can be added to the import from ./types/Param.types
.
*/ | ||
function getAge(inputFilename: string | undefined, name: string) { | ||
if (inputFilename) { | ||
const data = readJsonFromFile<any>(inputFilename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can type this as AgeDistributionData
instead of any
. That type can be added to the import from ./types/Param.types
.
this does look like a very useful direction and we might consider ditching our parallel implementation in python for model fitting. |
@adityasharad Thanks for reviewing. @adityasharad @bettbra I will merge and then fix the kinks on master. Let me know if there's anything to ammend or what other improvements you have in mind. |
Thanks all for the input. I fixed the eslint kinks on my branch without changing the eslint settings. I can open another pull request for it if you'd like; apologies for the delay. Thanks @adityasharad and @ivan-aksamentov for your feedback. |
Oh, and I should have commented @rneher: We're grateful for any direction you want to take the project that allows command line running, or more broadly programmatic modification of parameters. If this does the trick for the community, super, but happy to switch to any other language or methodology. Thanks again for all your hard work on this. |
Add a yarn target for running the model via the command line.
Description
These changes allow the model to be run via the command line via a
yarn cli
call. Two arguments are required:--scenario
and--out
.Impacted Areas in the application
The changes are all pretty trivial. Some command line parsing is done via
yargs
insrc/algorithms/cli.ts
(and so a dependency is introduced on the yargs package inpackage.json
).Testing
docs/comand_line.md
into a filescenario.json
.yarn cli --help
to see the usage message.yarn cli --scenario=scenario.json --out=output.json
.output.json
was created by the run and has reasonable-looking output.Additional Notes