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

faster make test #1538

Merged
merged 2 commits into from
Feb 6, 2024
Merged

faster make test #1538

merged 2 commits into from
Feb 6, 2024

Conversation

orichters
Copy link
Contributor

@orichters orichters commented Feb 1, 2024

Purpose of this PR

  • quicker tests (~ 13 min instead of 22 min)
  • instead of running a testOneRegi with action = c, run Rscript start.R --gamscompile. Is somehow much faster (2 min -> 30 seconds)
  • don't compile 27 AMT runs, but just a selection of 6 that should cover the modules that are used. Reduces runtime from 12 min to < 5 min. I added a EU21 run as requested below. It is time-consuming as new input data has to be downloaded and distributed twice (before and after the run)

Type of change

  • Refactoring

Checklist:

  • My code follows the coding etiquette
  • I performed a self-review of my own code
  • I explained my changes within the PR, particularly in hard-to-understand areas
  • All automated model tests pass (FAIL 0 in the output of make test)
  • The changelog CHANGELOG.md has been updated correctly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Adding a EU21 run is maybe advisable, but also time-consuming as new input data has to be downloaded and distributed twice (before and after the run)

Definitely test EU21. Download times are not an issue on the cluster, people can always test there, or cache locally if they are so inclined.
But changes in regional aggregation are a much more serious source of errors then different module realisations, in my experience.

@@ -183,11 +183,6 @@ if (any(! file.exists(c(path_settings_coupled, path_settings_remind))) ||
stop("Missing files or directories, see in red above.")
}

if ("--gamscompile" %in% flags && ! file.exists("input/source_files.log")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is not needed anymore, as runGamsCompile does that alone

@orichters orichters force-pushed the fasttest branch 2 times, most recently from ab3a186 to b82e9a3 Compare February 1, 2024 15:17
@orichters
Copy link
Contributor Author

Something went wrong with the rebase, not fixing it now as #1539 will lead to merge conflicts anyway so that has to be redone anyway.

@orichters
Copy link
Contributor Author

Ok, including the EU21 run, time is down from ~22 to 14 min.

@orichters
Copy link
Contributor Author

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q: Thanks for the comment, I added SSP2EU-EU21-Base to the list of scenarios to be compiled, maybe you can remove "changes requested".

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

Sure. I thought you planned on making a new merge request in order to not deal with the conflicts.

@orichters
Copy link
Contributor Author

Sure. I thought you planned on making a new merge request in order to not deal with the conflicts.

I just wanted to wait for #1536 and #1539 to be merged to avoid fixing the conflicts several times.

@orichters orichters merged commit 5264f73 into remindmodel:develop Feb 6, 2024
2 checks passed
@orichters orichters deleted the fasttest branch February 8, 2024 14:24
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