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

Aggiunta dataset custom_powerplants #1347

Closed
wants to merge 7 commits into from

Conversation

Asdominet34
Copy link

Closes # (if applicable).

Changes proposed in this Pull Request

Checklist

  • I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Newly introduced dependencies are added to envs/environment.yaml and doc/requirements.txt.
  • Changes in configuration options are added in all of config.default.yaml and config.tutorial.yaml.
  • Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
  • Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

Copy link
Member

@ekatef ekatef left a comment

Choose a reason for hiding this comment

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

Hello @ValeMTo and many thanks for the contribution! I see that you aim tailor the model for DRC, and that looks like a great work which resonates with our current priorities to increase accuracy for each part of the world.

Your implementation looks nice, and I really love some ideas you have put inside. Have added some comments which main goal is to make your contribution helpful for anyone using the code. Let me know please what you think!

The comments may look being too many but my feeling is that they can be tackled relatively easily. The main part is to align on the architecture, especially for the parts which relate to region-specific data. I'm also available for a discussion if you feel it can be useful.

Great work, and looking forward to accommodate your contribution 😄

Comment on lines +1 to +4
,Name,Fueltype,Technology,Set,Country,Capacity,Efficiency,Duration,Volume_Mm3,DamHeight_m,StorageCapacity_MWh,DateIn,DateRetrofit,DateOut,lat,lon,EIC,projectID,bus
1,Victoria Falls,Hydro,Run-Of-River,PP,ZM,108,,,0,0,0,1936,1936,2036,-17.9305,25.8598,,,
2,Zilo,Hydro,Reservoir,PP,CD,110,,,0,0,0,1952,1952,2052,-10.4998,25.4631,,,
3,Mwadingusha,Hydro,Reservoir,PP,CD,68,,,0,0,0,1928,1928,2028,-10.7452,27.2447,,,
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for sharing the data!

On the particular implementation, I fear there may be some technical issues... Usually, it's a not very good idea to git-track the data files as it can easily lead to an inflation of the repository

Though, we would love to have the data easily available, and a solution for that can be absolutely found. One possible solution would be to create a databundle, load it to Zenodo and add a rule to retrieve this dataset if the country is requested by a user (could be also beneficial as a part of dissemination work you are doing for your study)

Comment on lines +467 to +468
Processa i dati del file Excel e li combina con il DataFrame 'hydro'.
#fulmicotone
Copy link
Member

Choose a reason for hiding this comment

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

I love Italian but unfortunately it's against PEP8 coding standard to have comments in the language in any other language than English (as discriminative as it sounds...) Happy to support you with translation if needed

@@ -541,7 +662,28 @@ def attach_hydro(n, costs, ppl):
* hydro_inflow_factor
)

# fulmicotone
excel_path = r"C:\Users\Davide\pypsa-earth-project\pypsa-earth\data\African_Hydropower_Atlas_v2-0_PoliTechM.xlsx"
Copy link
Member

Choose a reason for hiding this comment

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

To make the workflow run for everyone, we need to provide the datafile. Just to make sure it's possible in principle: would opening the data be complaint with the license restrictions?

Comment on lines +672 to +673
print(p_max_pu_ror.shape) # (numero_di_righe_di_final_data, 8760)
print(p_max_pu_ror.head()) # Anteprima dei dati
Copy link
Member

Choose a reason for hiding this comment

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

I guess calls of print() have been needed for debug which it perfectly reasonable. However, those calls should not go to the codebase. If you think it's worth to provide a user with some information please use logging functionality for that

@@ -462,6 +462,124 @@ def attach_conventional_generators(
n.generators.loc[idx, attr] = values


def process_hydropower_data(excel_path, hydro, sheet_name):
Copy link
Member

Choose a reason for hiding this comment

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

I guess that is a custom function to work with African Hydropower Atlas, right? If that's the case it could be probably designed as a dedicated rule to produce a csv-file of custom powerplants data. What do you think?

A placeholder for such a file is available in data folder and gives a hint on the data structure expected by the workflow

Comment on lines +184 to +185
def calculate_scale(gegis_load, file_path_temba):
"""
Copy link
Member

Choose a reason for hiding this comment

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

A great idea on having in-built calculation of the demand scaling factor! The major comment on that is that it's crucial to make the approach applicable for any country of the world. Would you be interested in that?

Comment on lines +211 to +221
# Leggi la pagina "SpecifiedAnnualDemand" del file Excel
df_temba = pd.read_excel(file_path_temba, sheet_name="SpecifiedAnnualDemand")

# Rimuovi eventuali spazi all'inizio e alla fine dei nomi delle colonne
df_temba.columns = df_temba.columns.astype(str).str.strip()

# Crea una nuova colonna chiamata 'Country' contenente solo le prime due lettere della colonna 'FUEL'
df_temba["Country"] = df_temba["FUEL"].str[:2]

# Rimuovi la colonna originale 'FUEL'
df_temba = df_temba.drop(columns=["FUEL"])
Copy link
Member

Choose a reason for hiding this comment

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

If feels like this part can be wrapped-up into a dedicated function to parse TEMBA data. It would improve readability of the code and also allow to an easy generalisation by replacing TEMBA-parser with one relevant for a particular region

Comment on lines +244 to +245
print("scale")
print(scale)
Copy link
Member

Choose a reason for hiding this comment

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

Mind the prints ;)

@@ -187,6 +257,7 @@ def build_demand_profiles(
start_date,
end_date,
out_path,
file_path_temba, # fulmicotone
Copy link
Member

Choose a reason for hiding this comment

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

Best would be to add file_path_temba to Snakemake inputs for build_demand_profile rule. It allows for more flexible and clean file management

Comment on lines +394 to +396
# fulmicotone
file_path_temba = r"C:\Users\Davide\Downloads\temba\TEMBA_SSP1-26.xlsx"
# year = 2030
Copy link
Member

Choose a reason for hiding this comment

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

To clarify the above comment on Snakemake inputs: it's currently not obvious where file_path_temba is defined, and super easy to forget modify it if anything has been changed. Having this specification in Snakemake would facilitate keeping everything in sight

@ValeMTo
Copy link

ValeMTo commented Feb 13, 2025

Thank @ekatef so much for reviewing our pull request and as soon as we can, we will fix the issues that you highlighted.
This is a kind of spoiler of what we are working on since we mistakenly made this pull request!

Would it be better to cancel this PR and make a new one once we are ready?

@ekatef
Copy link
Member

ekatef commented Feb 13, 2025

Thank @ekatef so much for reviewing our pull request and as soon as we can, we will fix the issues that you highlighted. This is a kind of spoiler of what we are working on since we mistakenly made this pull request!

Would it be better to cancel this PR and make a new one once we are ready?

Ahhh, sorry!! I though you are seeking for the feedback 😄 No worries, and the great work, in any case!

Sure, feel free to close this PR 🙂 On the technical side, it's usually easier when a PR deals with a single task. For example, it makes sense to open one PR on adding calculations for the scaling factors and another one on adding custom time-series for the runoff. In case you would be interested to contribute those features, it would be very much appreciated!

@ValeMTo
Copy link

ValeMTo commented Feb 14, 2025

Don't worry, you were right to think that we were looking for feedback. Indeed, we made a PR lol
We want to contribute with these features :)
We will open single-task PRs!

Thanks and sorry again!!

ValeMTo added a commit to ValeMTo/pypsa-earth that referenced this pull request Feb 14, 2025
@ValeMTo ValeMTo deleted the aggiunta-csv branch February 14, 2025 14:16
@ekatef
Copy link
Member

ekatef commented Feb 14, 2025

Don't worry, you were right to think that we were looking for feedback. Indeed, we made a PR lol We want to contribute with these features :) We will open single-task PRs!

Thanks and sorry again!!

Good luck with your modeling study, and looking forward to accommodate your contributions when it'll be a right time for it 😄

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.

3 participants