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

Fix naming of GDP and population columns in SSP data aggregation within tools.costs #219

Merged
merged 5 commits into from
Sep 2, 2024

Conversation

measrainsey
Copy link
Contributor

@measrainsey measrainsey commented Aug 21, 2024

Small fix to total_gdp and total_population columns being switched in message_ix_models.tools.costs.gdp.process_raw_ssp_data()

In the message_ix_models.tools.costs.gdp.process_raw_ssp_data() function that pulls and combines SSP data in tools.costs, there is a block of code that converts the data in the Computer() into a pandas DataFrame, with each computer key being a column. However, the naming of the columns are switched (total_population and total_gdp). So, I just swapped them.

From what I can tell, this has zero impact on the actual costs calculations, as these DataFrames columns are not used anywhere (and are later even dropped). The calculation of GDP per capita happens before this DataFrame is created, here:

# GDP per capita
c.add(k_gdp["cap"], "div", k_gdp.base, k_pop)

And these values seem to be correct.

I picked up on this because I am calling using the output of message_ix_models.tools.costs.gdp.process_raw_ssp_data() function on its own elsewhere.

How to review

For @khaeru and/or @glatterf42 : Read the diff and note that the CI checks all pass.

PR checklist

  • Continuous integration checks all ✅
  • Update doc/whatsnew.

@measrainsey measrainsey self-assigned this Aug 21, 2024
@khaeru
Copy link
Member

khaeru commented Aug 21, 2024

I don't recall precisely, but I would guess that I put the total_population and total_gdp columns in place during work on #99 because you had other code further downstream that appeared to be using those data.

If that's since changed, and they're no longer needed, a better fix would be to remove them and the associated code entirely.

@measrainsey
Copy link
Contributor Author

I don't recall precisely, but I would guess that I put the total_population and total_gdp columns in place during work on #99 because you had other code further downstream that appeared to be using those data.

If that's since changed, and they're no longer needed, a better fix would be to remove them and the associated code entirely.

Thanks Paul -- I am using the all of the columns in the output of message_ix_models.tools.costs.gdp.process_raw_ssp_data() (to make figures for writing/presenting), so for me it would be nice to keep the columns in there, even if they're not used for the module itself. But if it's causing too much clutter in the module code then we can also consider removing this part.

@khaeru
Copy link
Member

khaeru commented Aug 29, 2024

I am using the all of the columns

Okay, then good to keep them. In that case I'd instead ask to expand the docs with half a sentence for these two columns, explaining precisely what is contained in each, so that others can maybe use them in the same way.

@khaeru
Copy link
Member

khaeru commented Aug 29, 2024

The CI failures can likely fixed by cherry-picking 9e565c9 from #213.

@khaeru khaeru added the costs `.tools.costs`/cost data preparation label Aug 29, 2024
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 58.8%. Comparing base (cf687da) to head (33aa238).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
message_ix_models/tools/wb.py 0.0% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #219     +/-   ##
=======================================
- Coverage   58.8%   58.8%   -0.1%     
=======================================
  Files        194     194             
  Lines      15159   15161      +2     
=======================================
  Hits        8922    8922             
- Misses      6237    6239      +2     
Files with missing lines Coverage Δ
message_ix_models/tools/costs/gdp.py 94.6% <ø> (ø)
message_ix_models/tools/wb.py 29.8% <0.0%> (-0.7%) ⬇️

Copy link
Member

@khaeru khaeru 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 not sure why the .tools.wb cherry-picked changes are apparently not covered by the tests, since the corresponding tests do seem to run (although XFAIL).

However, LGTM and then continue with other work/clean-up, e.g. in #213 from which those changes were cherry-picked.

@khaeru khaeru merged commit 5fe36ca into main Sep 2, 2024
25 of 26 checks passed
@khaeru khaeru deleted the costs/ssp-fix branch September 2, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
costs `.tools.costs`/cost data preparation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants