search

openego / eGon-data

GNU Affero General Public License v3.0
10 stars 4 forks source link

Fix DSM time series #1089

Closed khelfen closed 1 year ago

khelfen commented 1 year ago

Fixes #1088 .

Before merging into dev-branch, please make sure that

khelfen commented 1 year ago

@ClaraBuettner / @IlkaCu this PR is completed from my side. Is there any CI run planned? In which CI branch should I merge those changes for testing?

IlkaCu commented 1 year ago

You can use CI branch continuous-integration/run-everything-2022-11-10. But we didn't schedule the new CI run yet.

khelfen commented 1 year ago

@KathiEsterl maybe you could already review this PR?

KathiEsterl commented 1 year ago

Do you think it is necessary to run the code and check the results again? I guess you have done it anyways.

I would suggest to approve it, although I "only" looked at the changes in the code one more time and checked the code together with you yesterday. I think for those changes this is sufficient.

khelfen commented 1 year ago

Do you think it is necessary to run the code and check the results again? I guess you have done it anyways.

I would suggest to approve it, although I "only" looked at the changes in the code one more time and checked the code together with you yesterday. I think for those changes this is sufficient.

The only thing I can imagine is that the chosen tolerance for the sanity checks is not high enough in all cases in a full run. That way the sanity checks would fail, but everything should still work as expected.

https://github.com/openego/eGon-data/blob/9b94154ceb07ec75db6d71b1463ebeae0b2cc1e7/src/egon/data/datasets/sanity_checks.py#L1488-L1494

I'm not sure if that should be tested within the CI before merging. Would be an easy fix anyway. @IlkaCu maybe you can decide this?

KathiEsterl commented 1 year ago

It has been running on CI meanwhile, right? How is it about the tolerances? If everything is fine, we can merge it to dev now, I guess.