-
Notifications
You must be signed in to change notification settings - Fork 130
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
reactive check_config, add regexp check for cfg$… values #1356
Conversation
a02001a
to
33e0dc2
Compare
@@ -59,6 +59,10 @@ test-coupled: ## Test if the coupling with MAgPIE works. Takes significantly | |||
$(info Coupling tests take around 75 minutes to run, please be patient) | |||
@R_PROFILE_USER= TESTTHAT_RUN_SLOW=TRUE Rscript -e 'testthat::test_file("tests/testthat/test_20-coupled.R")' | |||
|
|||
test-coupled-slurm: ## test-coupled, but on slurm |
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.
Shouldn't test-coupled be removed from make commands so you can only run coupled test with SLURM?
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 don't think so. The model runs are on slurm anyway, the difference is whether also the test itself is on slurm. I think normally you should not do that (because it reserves a full CPU basically for waiting), but sometimes if the internet connection is unstable (train…) it is a helpful option because then the tests do not fail just because your console lost connection.
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.
Great, thanks for adding the comments. That helps. Could you add one ore two sentences at the top of the function that explain what the funtion does and what it is used for in general? Thank you.
Purpose of this PR
checkFixCfg()
, with reading the config from main.gms, and adding all the other changes to cfg that were inprepare.R
main.gms
where the parameter or setglobal value is specified, addregexp = whatever
which is used to check the value used for this switch. This allows to catch wrong settings for parameters such ascm_emiscen
where no other such check existed, yet. There are certainly other lines inmain.gms
where this can be added, but it is a start…checkFixCfg()
instart.R
andstart_bundle_coupled.R
such that errors are caught early, already while running in--test
modestart.R --test
such that errors are caught in the tests already. This costs less than 4 minutes, but at the moment it is skipped in the quick testsscenario_config_coupled
files andstart_bundle_coupled.R --test
which is run usingmake test-coupled
, adding < 2 minutes to the 75 it had before.make test-full
mode which takes hours anyway. Takes about 75 minutes, but I think that is worth a full test.gms::getLine
such that they fail if that happens, instead of just waiting for nothing to happen.startgroup=*
such thatselectScenarios()
starts everythingIn the process, the new tests lead to finding some bugs that I fixed in #1355 and #1359
Type of change
Checklist:
remind2
was not neededFAIL 0
in the output ofmake test
,make test-coupled
,make test-full
)