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

Adaptations to industry demand #317

Merged
merged 26 commits into from
Jul 19, 2024
Merged

Conversation

hazemakhalek
Copy link
Collaborator

Closes #315

Changes proposed in this Pull Request

The PR further enhances the carrier aggregation functions by sourcing the needed data from the helpers.py file.
The PR also redistributes the fuels to the right carriers, with bio now reflecting biofuels in general rather than only biomass.

Checklist

  • 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 envs/environment.docs.yaml.
  • Changes in configuration options are added in all of config.default.yaml, config.tutorial.yaml, and test/config.test1.yaml.

@@ -620,26 +620,22 @@ def get_conv_factors(sector):
"Gas Coke": 0.007326,
"Refinery gas": 0.01375,
"Coal Tar": 0.007778,
"Paraffin waxes": 0.01117,
"Ethane": 0.01289,
"Oil shale": 0.00247,
}
return fuels_conv_toTWh


def aggregate_fuels(sector):
Copy link
Contributor

@finozzifa finozzifa May 29, 2024

Choose a reason for hiding this comment

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

Hi @hazemakhalek,

thanks a lot for this great PR.

Problem I see

I noticed that not all the fuels available in get_conv_factors() are present in aggregate_fuels(). Moreover there are also some fuels available in aggregate_fuels() which are not present in get_conv_factors() .

Why I think it is a problem

My understanding of the code is that what appears in get_conv_factors() should be also "mapped" in aggregate_fuels().

In build_base_industry_totals.py we have in fact the dictionary fuel_dict being built as follows

    # Fetch the fuel categories from the helpers script
    (
        gas_fuels,
        oil_fuels,
        biomass_fuels,
        coal_fuels,
        heat,
        electricity,
    ) = aggregate_fuels("industry")

    # Create fuel dictionary to use for mapping all fuels to the pypsa representative fuels
    fuel_dict = {
        element: var_name
        for var_name, element_list in [
            ("gas", gas_fuels),
            ("oil", oil_fuels),
            ("biomass", biomass_fuels),
            ("heat", heat),
            ("coal", coal_fuels),
            ("electricity", electricity),
        ]
        for element in element_list
    }

Such dictionary is then used as to populate the "Carrier" column. Namely:

df["carrier"] = df["Commodity"].map(fuel_dict)

Of course, it could be that my understanding of the code is incorrect. In this case I will drop this review.

What I propose to enrich

The fuels available in get_conv_factors(), but not present in aggregate_fuels() are:

'Additives and Oxygenates', 
'Refinery Gas', 
'Coal Tar', 
'Brown Coal Briquettes', 
'Gas coke', 
'Aviation gasoline',
'Gasoline-type jet fuel', 
'Peat Products', 
'Oil shale', 
'Bio jet kerosene', 
'Paraffin waxes'

The fuels available in aggregate_fuels(), but not present in get_conv_factors() are

'Natural gas (including LNG)', 
'Coke Oven Gas', 
'Natural Gas (including LNG)', 
'Black Liquor', 
'Biogases', 
'Gasworks Gas', 
'Blast Furnace Gas'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @finozzifa ,

Thanks for the quick review! You're interpretation is correct.

I tested it quickly now and the first part of your suggestion is correct and need to be added. As of now, these fuels are calculated but not added to the aggregate value of the representative fuel. (e.g. Aviation gasoline is not added to Oil)

The second part however is intended, the fuels you show here are given in the dataset in energy units already (Terajoules)
These are handled on the line;

df.loc[index_energy_TJ, "Quantity_TWh"] = df.loc[index_energy_TJ].apply( lambda x: x["Quantity"] / 3600, axis=1)

The fuels conversion factors are only used to convert from mass or volume to energy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @hazemakhalek ,

thanks for your answer and for your explanation :)

In order words, we should include all entries from get_conv_factors() to aggregate_fuels(), whereas it is fine if aggregate_fuels() misses some entries available in get_conv_factors().

Did I get it right?

speak soon,

Fabrizio

Copy link
Contributor

@finozzifa finozzifa May 30, 2024

Choose a reason for hiding this comment

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

Answer from PyPSA-Earth-Sec meeting on May 30th 2024

All fuels should be present in aggregate_fuels(), but not all fuels should be in get_conv_factors().

Thanks @hazemakhalek

Copy link
Member

Choose a reason for hiding this comment

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

Great discussion!
To add, some factors in the list may be typos or refuses; for simplicity, the naming may be revised to be normalized and maybe avoid duplicated entries that may introduce noise.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey Davide, I agree with you in general.
The typos in the data could be handled in a more consistent way to correct the trailing spaces, typos of the source.
If you handle this in a specific way for the power model let me know so we can make it coherent.

If not, I will start an issue which will be good for as an early task for some new comers.

Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

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

Great:D
Added my 5 cents :) maybe good to discuss tomorrow?!?!

scripts/build_base_energy_totals.py Show resolved Hide resolved
@@ -113,12 +132,16 @@ def calc_sector(sector):
sectors_dfs[sector] = df_sector.copy()

if sector == "consumption by households":
if snakemake.config["sector"]["coal"]["shift_to_elec"]:
Copy link
Member

Choose a reason for hiding this comment

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

It seems to be that here as well operations may be similar and may be aggregated.
For example, if we define a dictionary like:

d_config = {
"consumption by households": ["Electricity"],
"services": ["oil", "biomass", ...]
...
}

And we also standardize the oil_fuels/biomass_fuels etc in an automated way, the script can be significantly improved.
Probably worth to keep in mind this and add an issue about it?
Revising this script can be quite heavy and I don't think it is a priority

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think an issue here will be useful. Again, I think this is an interesting task for early users especially with software background

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added to issue: #358

@@ -620,26 +620,22 @@ def get_conv_factors(sector):
"Gas Coke": 0.007326,
"Refinery gas": 0.01375,
"Coal Tar": 0.007778,
"Paraffin waxes": 0.01117,
"Ethane": 0.01289,
"Oil shale": 0.00247,
}
return fuels_conv_toTWh


def aggregate_fuels(sector):
Copy link
Member

Choose a reason for hiding this comment

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

Great discussion!
To add, some factors in the list may be typos or refuses; for simplicity, the naming may be revised to be normalized and maybe avoid duplicated entries that may introduce noise.

What do you think?

scripts/helpers.py Show resolved Hide resolved
@hazemakhalek hazemakhalek marked this pull request as ready for review June 24, 2024 15:03
@@ -243,7 +288,64 @@ def calc_sector(sector):
].Quantity_TWh.sum(),
4,
)
elif sector == "other energy":
Copy link
Member

Choose a reason for hiding this comment

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

These part may be improved (now or in the future) to be more compact code-wise.

Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

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

Great @hazemakhalek :D
I've added a recommendation and then for me the PR is ready to fly

scripts/build_base_industry_totals.py Outdated Show resolved Hide resolved
@hazemakhalek
Copy link
Collaborator Author

@davide-f Done, please merge the PR

@davide-f davide-f merged commit b7da641 into main Jul 19, 2024
4 of 5 checks passed
@davide-f
Copy link
Member

Great :D done :)

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.

Duplication in build_base_energy_total.py
3 participants