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

use new compareScenarios2() in test instead of old compareScenarios() #249

Merged
merged 2 commits into from
May 5, 2022

Conversation

chroetz
Copy link
Contributor

@chroetz chroetz commented May 4, 2022

See #238.

@chroetz
Copy link
Contributor Author

chroetz commented May 4, 2022

On my machine, the test now takes 3min.

@chroetz chroetz marked this pull request as ready for review May 4, 2022 10:20
@orichters
Copy link
Contributor

orichters commented May 4, 2022

Thanks, Christof! May it be worth adding a second fulldata.gdx, maybe a H21 run (if that is possible in a cs2 to have both simultaneously?), or with sector-differentiated carbon prices (would have catched this one), from a coupled run or something like that? So to say, design a second mif which is maximally different from the current one?

And have you thought about #172?

@orichters orichters linked an issue May 4, 2022 that may be closed by this pull request
@Renato-Rodrigues
Copy link
Member

Could the library tests for alternative gdxs be done in parallel? Maybe @giannou already know about that?
If not I am against adding additional gdx files to be tested for now.
Current tests already take too long, so I would first focus on improving test performance/testing single regions instead, and so on, before making the accumulated number of tests take a very long time to finish.

@orichters
Copy link
Contributor

@Renato-Rodrigues: As far as I understand, @chroetz managed to bring the test time down to 3 minutes (convGDX2mif + cs2), so even if we add a second gdx, it would still be a big improvement.

@orichters orichters changed the title use new compareScenarios2() in test instead of old compareSceanrios() use new compareScenarios2() in test instead of old compareScenarios() May 4, 2022
@Renato-Rodrigues
Copy link
Member

The problem is keep summing up tests above tests without worrying about the total buildLibrary time.

If tests can run in parallel, I completely agree with adding as many tests as we can.

I am not ok with having a lot of tests added sequentially because we need then to wait half hour or more for the buildLibrary to finsih to be able to commit an urgent model breaking change.
Maybe I am biased by the fact that when things get to me they are usually too urgent and need to be fixed asap, but this framework works very nicely for non urgent cases, but it is unbearable for cases like the one I am describing here.

@orichters
Copy link
Contributor

Yes, I agree. So: If we don't provide a second gdx file, but just a second mif file (that then has to be updated regulary to keep track of the changes in the reporting), this shouldn't extend the runtime of cs2 at all.

@giannou
Copy link
Contributor

giannou commented May 4, 2022

We used to have parallelization in the tests, we can maybe reintroduce it. I think it was removed for reasons that are not relevant any more.

@Renato-Rodrigues
Copy link
Member

gdx files are not updated as frequently as mif files. We add or change reporting variables more often than model variables. So I don't think the mif file is a good approach.
I think the solution is to make sure that tests can run in parallel for multiple gdxs and scenario comparison tests. I am not even sure if this is the case already as I never checked the tests code.

I think we are going to far from the topic of the PR, maybe we should discuss that in an issue instead.

@chroetz
Copy link
Contributor Author

chroetz commented May 4, 2022

  1. I timed the code again in more detail: I ran the test three times and it was always roughly 120s (Don't know why it was 180s before...)
  • 85s convGDX2MIF()
  • 25s checkIntegrity()
  • 7s compareScenarios2()
  • 3s other stuff
  1. Should we parallelize using parallel::mclapply()?

  2. Currently the test downloads https://rse.pik-potsdam.de/data/example/remind2_test-convGDX2MIF_fulldata.gdx and https://rse.pik-potsdam.de/data/example/historical.mif. Are these files maintained / updated? How can additional gdxs be added?

@orichters
Copy link
Contributor

I suggest to merge that now because it is already a big relief, and discuss further which gdx files could be added, if this was desired.

Copy link
Contributor

@orichters orichters left a comment

Choose a reason for hiding this comment

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

I took the test and ran it without deleting the file and had a look at it. Seems to work and is much faster than before.

@chroetz chroetz merged commit 77362f7 into pik-piam:master May 5, 2022
@chroetz chroetz deleted the adapt-test branch July 27, 2022 13:19
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.

adapt test-convGDX2mif.R to compareScenarios2
5 participants