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

Features/heat load #76

Merged
merged 72 commits into from
Feb 28, 2022
Merged

Features/heat load #76

merged 72 commits into from
Feb 28, 2022

Conversation

MaGering
Copy link
Collaborator

@MaGering MaGering commented Oct 13, 2021

Resolves #53 by adding a function that calculates heat load profiles.

Hence the demandlib is used to calculate profiles for households and GHD, the install_requires in setup.py and snakemake rule in snakemake file are updated as well.

@MaGering MaGering self-assigned this Oct 13, 2021
Copy link
Collaborator

@jnnr jnnr 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 work! Looks very good already. Here are some comments - haven't tested yet.

Snakefile Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jnnr jnnr 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 work, @MaGering! Looks pretty good already. Some things can be cleaned up a bit still (see the individual comments).

scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
Snakefile Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
Snakefile Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
@MaGering MaGering requested a review from jnnr November 22, 2021 18:10
Copy link
Collaborator

@jnnr jnnr 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, @MaGering! Much of it looks quite good and is on the right track. Some things can be improved and should be improved - see my suggestions. Please make sure that the script runs successfully - I tried to test it but it failed to complete.

scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
scripts/prepare_heat_demand.py Outdated Show resolved Hide resolved
@jnnr jnnr self-requested a review February 21, 2022 12:34

if __name__ == "__main__":
in_path1 = sys.argv[1] # path to weather data
in_path2 = sys.argv[2] # path to household distributions data
Copy link
Collaborator

Choose a reason for hiding this comment

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

Household distribution data is not used?

Copy link
Collaborator Author

@MaGering MaGering Feb 22, 2022

Choose a reason for hiding this comment

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

You're right. Fixed with commit c07bde4 and 8758456.

@MaGering MaGering requested a review from jnnr February 22, 2022 13:37
total_heat_load = postprocess_data(
total_heat_load, heat_load_year, region, f"ts_{year}", sc_demand_unit
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a function here that sums up the scalars for hh, ghd and i?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done with commit a8a7b1f and a8a7b1f with potential for more efficient and cleaner coding. Also, I don't know how to get rid of the SettingWithCopyWarning which occurs when I intentionally overwrite values in a DataFrame.

@jnnr
Copy link
Collaborator

jnnr commented Feb 25, 2022

@MaGering: Can you please have a final check to make sure that the prepared scalars and timeseries are correct?

After that, the filenames have to be cleaned up - then we can merge.

@MaGering
Copy link
Collaborator Author

Thanks for the adjustments! It runs through and the outputs look good to me.

Should I change results/_resources/scal_load_heat.csv to results/_resources/scalars.csv. Or what exactly did you have in mind?

@jnnr
Copy link
Collaborator

jnnr commented Feb 28, 2022

Thanks for the adjustments! It runs through and the outputs look good to me.

Should I change results/_resources/scal_load_heat.csv to results/_resources/scalars.csv. Or what exactly did you have in mind?

No, the naming should stay - the script now produces both scalars and timeseries.

@jnnr jnnr merged commit ab4d1c1 into dev Feb 28, 2022
@jnnr jnnr deleted the features/heat_load branch February 28, 2022 17:22
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.

Heat demand profiles are missing
2 participants