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

reactive check_config, add regexp check for cfg$… values #1356

Merged
merged 4 commits into from
Jul 21, 2023

Conversation

orichters
Copy link
Contributor

@orichters orichters commented Jul 12, 2023

Purpose of this PR

  • using gms::check_config was disable while moving settings to main.gms. Reenable it as checkFixCfg(), with reading the config from main.gms, and adding all the other changes to cfg that were in prepare.R
  • Add a regexp check for cfg$gms$ values: In the line of main.gms where the parameter or setglobal value is specified, add regexp = whatever which is used to check the value used for this switch. This allows to catch wrong settings for parameters such as cm_emiscen where no other such check existed, yet. There are certainly other lines in main.gms where this can be added, but it is a start…
  • call checkFixCfg() in start.R and start_bundle_coupled.R such that errors are caught early, already while running in --test mode
  • add to the tests that all scenario_config files are run with start.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 tests
  • Same with scenario_config_coupled files and start_bundle_coupled.R --test which is run using make test-coupled, adding < 2 minutes to the 75 it had before.
  • Compile every scenario from the config files, but only in make test-full mode which takes hours anyway. Takes about 75 minutes, but I think that is worth a full test.
  • Improve the tests that were calling scripts that potentially had gms::getLine such that they fail if that happens, instead of just waiting for nothing to happen.
  • add startgroup=* such that selectScenarios() starts everything

In the process, the new tests lead to finding some bugs that I fixed in #1355 and #1359

Type of change

  • Refactoring
  • New feature
  • This change requires a documentation update

Checklist:

  • My code follows the coding etiquette and extends it
  • I performed a self-review of my own code
  • I explained my changes within the PR, particularly in hard-to-understand areas
  • I checked that the in-code documentation is up-to-date
  • adjusted the reporting in remind2 was not needed
  • All automated model tests pass (FAIL 0 in the output of make test, make test-coupled, make test-full)

@orichters orichters marked this pull request as ready for review July 17, 2023 14:22
@orichters orichters requested review from fbenke-pik and removed request for LaviniaBaumstark July 17, 2023 14:24
@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@dklein-pik dklein-pik left a 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.

@orichters orichters merged commit f0754b1 into remindmodel:develop Jul 21, 2023
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants