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

OnSSET variables #320

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

OnSSET variables #320

wants to merge 19 commits into from

Conversation

mvittorio-unh
Copy link
Collaborator

New variables for OnSSET

Copy link
Collaborator

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mvittorio-unh for the very comprehensive pull request.

A couple of general points from my side:

  • The blank lines between variables are not needed. You can remove them for consistency with the rest of the template.
  • The variables that contain Baseline or End year are not necessary in my opinion since we report annual data anyway. If I understand your variable definitions correctly Baseline and End year would simply be the first and last value in your timeseries.
  • Out of convention, the unit for money we typically use is US$2010 (example: https://github.com/IAMconsortium/legacy-definitions/blob/d55beb19b80368d0676261da678ede296b9b615c/definitions/variable/variable.yaml#L524-L532). This would need to be changed for all your units involving money. You are obviously free to adjust the year for the dollar value that your model uses, e.g. US$2020.
  • The unit people I would suggest either replacing with unti: million or going fully dimensionless, i.e. unit: .

Please find some more comments in line below.

definitions/variable/economy/economy.yaml Outdated Show resolved Hide resolved
definitions/variable/economy/economy.yaml Outdated Show resolved Hide resolved
definitions/variable/economy/economy.yaml Outdated Show resolved Hide resolved
definitions/variable/economy/economy.yaml Outdated Show resolved Hide resolved
mappings/onsset.yaml Outdated Show resolved Hide resolved
definitions/variable/energy/energy-services.yaml Outdated Show resolved Hide resolved
definitions/variable/technology/electricity-expansion.yaml Outdated Show resolved Hide resolved
at the end of the study.
unit: us$

- Population|Connected|Total:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can leave out the total and rename the variable to:

Suggested change
- Population|Connected|Total:
- Population|Connected:

the total is implied by the hierarchy with the | character.

definitions/variable/technology/electricity-expansion.yaml Outdated Show resolved Hide resolved
definitions/variable/technology/electricity-grid.yaml Outdated Show resolved Hide resolved
@phackstock
Copy link
Collaborator

@mvittorio-unh, I fixed some of the issues and now we're getting close to finishing this PR.
Couple of issues to get through but then we should be done

Re-defined variables

Currently you re-defined 8 variables. This is an issue since a variable must only be defined once. These are the 8 in question:

ValueError: Collected 8 errors:
  1. Duplicate item in variable codelist: Population|Electricity access|Share
  2. Duplicate item in variable codelist: Network|Electricity|Expansion Cost
  3. Duplicate item in variable codelist: Population|Connected|Grid|Existing
  4. Duplicate item in variable codelist: Population|Connected|Grid|New
  5. Duplicate item in variable codelist: Population|Connected|PV
  6. Duplicate item in variable codelist: Population|Connected|Hybrid|PV
  7. Duplicate item in variable codelist: Population|Connected|Hybrid|Wind
  8. Duplicate item in variable codelist: Population|Connected|Hydro

I could just remove the ones that you added but at least for some of them you use different units to what was already there. Please take a look to make sure that your model outputs conform to the variables and remove your variables accordingly.

End Year variables

There are still a number of variables that contain End Year. From my understanding that would just be the last value in the time series. If that's correct please remove these variables. If my assumption is not correct, just let me know.

skip-region-aggregation: true

There are a number of variables that you introduced where you put skip-region-aggregation: true. Is that on purpose? What this attributes means that this variable will be skipped for region aggregation. As we're currently implementing African regions (West Africa, North Africa, etc...) we would want to aggregated as many variables as we can. So if you remove this line our processing will attempt to aggregate the variable.

Non-sum aggregations

There are a number of variables that are maximum or minimum values. These need special aggregation as the default is to just sum values. I'd be happy to help with that if you provide me with a list of variables that you'd like to add where you would say that a summation is not the right way to compute a meaningful aggregate.

Open comments

I have closed all the comments that you addressed from my last review. However, there's still a few open ones left. Please have a look, you can answer them in the comments directly.

@phackstock
Copy link
Collaborator

phackstock commented Apr 5, 2024

@mvittorio-unh, thanks for your answers via email. I've addressed all the open points mentioned above.
For the duplicate variable of Network|Electricity|Expansion Cost, I've taken the liberty and renamed it to Network|Electricity|Total Expansion Cost. I hope that's fine with you.
Unfortunately, we have a new (hopefully final) set of errors:

The following variables are used as 'weight' for aggregation but are not defined in the variable codelist:
'Final Energy|Diesel' used for 'Price|Final Energy|Diesel|Low' in: variable/energy/energy-prices.yaml
'Final Energy|Diesel' used for 'Price|Final Energy|Diesel|High' in: variable/energy/energy-prices.yaml
'Primary Energy|Electricity' used for 'Price|Primary Energy|Electricity' in: variable/energy/energy-prices.yaml
'Primary Energy|Electricity' used for 'Price|Primary Energy|Electricity|Expansion' in: variable/energy/energy-prices.yaml

these errors mean that as an aggregation weight you've used a variable that does currently not exist.
There are two possible fixes for this solution:

  1. You take a look at the current list of variables and find one that most closely matches the weight that you would want to use
  2. You add the missing variables to the codelists

Option 1. is preferable for me since it does not introduce new variables. If you can't find a good fit though adding new variables is totally fine as well.

@dc-almeida
Copy link
Collaborator

@mvittorio-unh @phackstock removed the variables as per the email, and resolved a merge conflict with the original repo regarding economy.yaml variables (the fork was behind main). Please double-check it's all good there.

Copy link
Collaborator

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Region aggregation needs to be skipped for a number of variables.
See reasoning in-line below.

skip-region-aggregation: true
- Capital Cost|Batteries:
description: Expected hybrid mini-grid component costs batteries.
unit: US$2020/kWp
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to set skip-region-aggregation: true here. Reason being that the default region aggregation simply adds up all the values for across different regions. For a variable with a unit like US$2020/kWp this would lead to wrong values.
We do offer the possibility of using a weighted average but for now, for simplicity let's skip region aggregation.
The same logic applies for all variables with US$2020/kWp and US$2020/km as a unit.

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