oemof / demandlib

Creating heat and power demand profiles from annual values.
https://oemof.org
MIT License
54 stars 38 forks source link

Add VDI profiles based on lpagg #50

Open uvchik opened 2 years ago

uvchik commented 2 years ago

The basic code is taken from https://github.com/jnettels/lpagg but most of the code has been revised for modular use and to speed up the code. The lpagg package contains various functionality but only the VDI-profiles and reading the TRY data from DWD is used.

To test the new profiles use the vdi_profile_example in the examples section. The parameter of the houses are added within a loop. I used this to multiply the number of houses to check how long it would take to model e.g. 1000 houses. 1000 houses on an hourly base will take less than a minute, which shows that the draft is at least fast enough :racing_car:

Related to #36

Install this branch to test the code:

pip install https://github.com/oemof/demandlib/archive/features/add-vdi-from-lpagg

It is the first draft, I am open for concrete ideas.

jnettels commented 2 years ago

@uvchik thanks very much, especially the speed looks promising! Not iterating through rows does indeed help a lot :-) I was able to run your example. I have reduced it to a calculation for a single house, and I am still getting differences between my implementation and yours, even if they are very small. It comes down to me identifying e.g. 21. October 2017 in TRY04 as WWB, while your code yields WWH. The difference is the daily mean of the cloud cover (< or >5). This only happens for 4 days, I think.

I am not 100% sure yet, but I think it comes down to the following: At 15min frequency, Pandas df.resample("D").mean() will use the values between (including) 2017-10-21 00:00 and 2017-10-21 23:15 to give the mean of 2017-10-21. However, in the weather data, 2017-10-21 00:00 describes the weather conditions between 2017-10-20 23:00 and 2017-10-21 00:00. The value in 2017-10-21 00:00 therefore belongs to 2017-10-20. To put it more simply: By DWD definition, the weather data does not start at 2017-1-1 00:00, but 2017-1-1 01:00. You seem to ignore that and simply load the weather data, starting at 2017-1-1 00:00, which may actually solve those problems. It may very well be that your solution is correct, and my many workarounds lead to incorrect results. Whenever I think about this stuff, I have to double and tripple check.

Some more stuff that might need consideration:

There might be more stuff, but these are all minor issues I think. I need some time for more investigation, though, to see if the API fits all the needs that I can come up with.

uvchik commented 2 years ago
pep8speaks commented 2 years ago

Hello @uvchik! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2022-09-06 18:25:57 UTC
jnettels commented 2 years ago

Here are the notes about what we discussed:

Some more notes that came up while replacing my own implementation in lpagg with yours:

(Please tell me if I should do some of those implementations myself, or if you prefer to do it yourself.)

Further discussion:

jnettels commented 2 years ago

I took the liberty to make two changes based on my previous suggestions: Named column levels and the ability to set my own weather file path. These changes help me to use demandlib within lpagg.

I think now the most important remaining issue is the ability to define the summer_temperature_limit for each house. My other suggestions are just for convenience.

jnettels commented 2 years ago

I have not tested it yet, but thanks for adding the temperature limits. Though, since you removed my solution to define a custom weather file, I need to ask: Was it rude (or just inconvenient) of me to push to this branch, or did you not agree with my approach?

p-snft commented 2 years ago

As this is a draft, I marked it as such.

Side note: I really like the feature. It's very handy to have generation of time series for SH, DHW, and electricity consumption at hand.

uvchik commented 2 years ago

For me it is a bit rude to write your code into my approach without any comment. I already wrote improvements and got dozen of merge conflicts. If I know somebody else is working on the code I would push more often to avoid such problems.

I do have a different approach for the weather file so I think the best way would be to finish this PR with cleaning the code and adding some tests and then start a new PR for the weather data. That makes it easier to track changes and to focus the discussion. One reason for that is that the original approach is connect to the TRY data. I am fine with adding your own weather data but it must be clear that you really should know what you are doing.

jnettels commented 2 years ago

Please accept my apologies for pushing into this branch, then. While I did leave a small comment about it here, I see how it creates unnecessary merge conflicts. Afterwards I learned how to use the "Suggested Change" feature and will stick to that in the future. Doing a new PR for the weather data is fine with me, too.

jnettels commented 1 year ago

Hi there,

it has been quite a while, but I am still depending on this implementation with its awesome speed. Is there any chance to get it over the finish line and merge it? Is there something I can do to help?

The changes I require to make it work for me personally are added in my fork and affect the ability to load other weather data. Just leaving the link here for reference: https://github.com/jnettels/demandlib/tree/features/add-vdi-from-lpagg

p-snft commented 1 year ago

@jnettels: Honestly, I'm not really deep into demandlib. I'm willing to review and give feedback. If you have an improved version, you can file a PR against this branch.

jnettels commented 1 year ago

Thanks for your answer. My "improvements" concerning the weather data were already suggested, once with a rude (still sorry about that!) push to this PR, once with the "suggestion" feature. @uvchik already mentioned that he had a different approach for the weather data, so I do not think creating a new PR for that is helpful now.