Skip to content

Add VDI profiles based on lpagg #50

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

Merged
merged 95 commits into from
Mar 3, 2025
Merged

Add VDI profiles based on lpagg #50

merged 95 commits into from
Mar 3, 2025

Conversation

uvchik
Copy link
Member

@uvchik uvchik commented Apr 12, 2022

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 🏎️

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.

  • add tests
  • add documentation
  • clean code

@uvchik uvchik self-assigned this Apr 12, 2022
@jnettels jnettels mentioned this pull request Feb 19, 2025
3 tasks
@uvchik
Copy link
Member Author

uvchik commented Feb 27, 2025

Implementation of a leap year is still missing but everything else should be fine now. We may want to merge this and start a new PR just for implementing leap years.

@jnettels In my opinion it is not a good idea to create a second PR. I merged both but it cost me some time.

@uvchik uvchik marked this pull request as ready for review February 27, 2025 23:07
@uvchik uvchik requested review from p-snft and jnettels February 27, 2025 23:07
Copy link
Contributor

@jnettels jnettels left a comment

Choose a reason for hiding this comment

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

Thanks for your effort!
I wanted to give this feature another chance of merging, but I had to spend my time adding missing code, tests and docs in an efficient way. I needed a clean base to do that. Also after >2.5 years I could not assume there would still be interest in this specific PR. This PR is targeting v0.2dev, which has long been surpassed*, and the branch was out of sync with dev in many regards. So the most efficient way for me seemed to start a fresh branch and cherry-pick all the necessary commits, which worked fine. It produced a clean history, passed all tests and could have just been merged. For me it was the choice between doing it like this, or not contributing at all (because I could not handle all the changes unrelated to the actual new feature). But I do not mind removing my PR, as long as we get the feature merged.

Now for some feedback:

  • You removed the file_weather argument of Region() and I do not see an obvious way to use custom weather data. We could add a new function from_file() to Climate(), which from_try_data() could also use internally.
  • The docs need to be updated with usage of the Climate() class
  • My code supported houses where Q_Heiz_a, W_a or Q_TWW_a are not defined. I think this is important, because sometimes you may e.g. only want the heating profiles, but no electricity profile. With my proposed changes, those columns are returned as all NaN, which can easily be filtered by the user.
  • demandlib.ini and config.py are still present. You used them at one point to store the default values for summer_temperature_limit and winter_temperature_limit. Now it only contains test_entry = 5, which I assume is also unrelated to the VDI feature. Are these files actually used?
  • Footnote*: I am very confused. Is dev the actual dev branch, or v0.2dev? The latter has updates to the BDEW api (get_scaled_power_profiles() instead of get_profile()), which are now also mixed in with this PR, making it even more confusing...

Comment on lines +506 to +508
q_heiz_a = house["Q_Heiz_a"]
w_a = house["W_a"]
q_tww_a = house["Q_TWW_a"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Allow skipping any energy type

Suggested change
q_heiz_a = house["Q_Heiz_a"]
w_a = house["W_a"]
q_tww_a = house["Q_TWW_a"]
q_heiz_a = house.get("Q_Heiz_a", float("NaN"))
w_a = house.get("W_a", float("NaN"))
q_tww_a = house.get("Q_TWW_a", float("NaN"))

# The typical day calculation inherently does not add up to the
# desired total energy demand of the full year. Here we fix that:
for column in load_curve_house.columns:
q_a = house[column.replace("TT", "a")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Allow skipping any energy type

Suggested change
q_a = house[column.replace("TT", "a")]
q_a = house.get(column.replace("TT", "a"), float("NaN"))

Comment on lines +80 to +116
def from_try_data(self, try_region, hoy=8760):
if try_region not in list(range(1, 16)):
raise ValueError(
f">{try_region}< is not a valid number of a DWD TRY region."
)
fn_weather = os.path.join(
os.path.dirname(__file__),
"resources_weather",
"TRY2010_{:02d}_Jahr.dat".format(try_region),
)
weather = dwd_try.read_dwd_weather_file(fn_weather)
weather = (
weather.set_index(
pd.date_range(
datetime.datetime(2010, 1, 1, 0),
periods=hoy,
freq="h",
)
)
.resample("D")
.mean()
)
self.temperature = weather["TAMB"]

weather.loc[weather["CCOVER"] >= 5, "cloud_category"] = "B"
weather.loc[weather["CCOVER"] < 5, "cloud_category"] = "H"
self.cloud_coverage = weather["cloud_category"]
fn_energy_factors = os.path.join(
os.path.dirname(__file__),
"vdi_data",
"VDI_4655_Typtag-Faktoren.csv",
)
self.energy_factors = pd.read_csv(
fn_energy_factors,
index_col=[0, 1, 2],
).loc[try_region]
return self
Copy link
Contributor

Choose a reason for hiding this comment

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

Allow custom DWD weather data by exposing from_file().

Suggested change
def from_try_data(self, try_region, hoy=8760):
if try_region not in list(range(1, 16)):
raise ValueError(
f">{try_region}< is not a valid number of a DWD TRY region."
)
fn_weather = os.path.join(
os.path.dirname(__file__),
"resources_weather",
"TRY2010_{:02d}_Jahr.dat".format(try_region),
)
weather = dwd_try.read_dwd_weather_file(fn_weather)
weather = (
weather.set_index(
pd.date_range(
datetime.datetime(2010, 1, 1, 0),
periods=hoy,
freq="h",
)
)
.resample("D")
.mean()
)
self.temperature = weather["TAMB"]
weather.loc[weather["CCOVER"] >= 5, "cloud_category"] = "B"
weather.loc[weather["CCOVER"] < 5, "cloud_category"] = "H"
self.cloud_coverage = weather["cloud_category"]
fn_energy_factors = os.path.join(
os.path.dirname(__file__),
"vdi_data",
"VDI_4655_Typtag-Faktoren.csv",
)
self.energy_factors = pd.read_csv(
fn_energy_factors,
index_col=[0, 1, 2],
).loc[try_region]
return self
def from_try_data(self, try_region, hoy=8760):
if try_region not in list(range(1, 16)):
raise ValueError(
f">{try_region}< is not a valid number of a DWD TRY region."
)
fn_weather = os.path.join(
os.path.dirname(__file__),
"resources_weather",
"TRY2010_{:02d}_Jahr.dat".format(try_region),
)
self.from_file(fn_weather, try_region, hoy)
return self
def from_file(self, fn_weather, try_region, hoy=8760):
if try_region not in list(range(1, 16)):
raise ValueError(
f">{try_region}< is not a valid number of a DWD TRY region."
)
weather = dwd_try.read_dwd_weather_file(fn_weather)
weather = (
weather.set_index(
pd.date_range(
datetime.datetime(2010, 1, 1, 0),
periods=hoy,
freq="h",
)
)
.resample("D")
.mean()
)
self.temperature = weather["TAMB"]
weather.loc[weather["CCOVER"] >= 5, "cloud_category"] = "B"
weather.loc[weather["CCOVER"] < 5, "cloud_category"] = "H"
self.cloud_coverage = weather["cloud_category"]
fn_energy_factors = os.path.join(
os.path.dirname(__file__),
"vdi_data",
"VDI_4655_Typtag-Faktoren.csv",
)
self.energy_factors = pd.read_csv(
fn_energy_factors,
index_col=[0, 1, 2],
).loc[try_region]
return self

@uvchik uvchik changed the base branch from v0.2dev to dev February 28, 2025 21:38
@uvchik
Copy link
Member Author

uvchik commented Feb 28, 2025

Okay, I agree the PR is allready very complex so I see two things we can do:

  1. Discuss your issues as we did it last time. This may lead to a better PR but has the risk of a never ending PR (because of the high complexity).
  2. Merge this PR and fix the issues in small dedicated PRs. One PR for each topic. e.g. "add weather_file_parameter" or "make it possible to get NaN". I think the discussion will have a clearer focus and the difficult discussions will not block the simple ones.

I would strongly prefere the second variant. Nothing happend by chance and I have good reasons why I did what I did.

We will merge the PR to dev! It is not the master branch. So there is still a developer space to fix the issues in small PRs before it will published as a stable version.

Copy link
Contributor

@jnettels jnettels left a comment

Choose a reason for hiding this comment

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

I am fine solving my personal remaining issues as separate PRs.
This PR now covers lots more than the VDI feature, but if you are sure about all of those other changes, then you have got my approvement 👍

@uvchik
Copy link
Member Author

uvchik commented Mar 3, 2025

Because of the difficult merge conflicts it looks more as it is. But this is one more reason to merge it and start with a cleaner structure. Thank you for all you comments and contributions.

@uvchik uvchik merged commit 96214a5 into dev Mar 3, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cooperate with lpagg
4 participants