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

convertGDP(replace_NAs) does not work as intended/documented #36

Closed
0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q opened this issue Oct 10, 2024 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented Oct 10, 2024

I am puzzled by convertGDP(replace_NAs). It appears that convertGD(…, replace_NAs = "with_USA") is the default mode, at least within the mrremind package:

$ grep convertGDP\( ./R/*.R | wc -l
21
$ grep replace_NAs ./R/*.R | wc -l
20

But it does not opperate as advertised

"with_USA": missing conversion factors in the source object are replaced with
the conversion factors of the USA.

instead converting all values with the USA factors

toolGetMapping('regionmappingH12.csv') %>% 
  as_tibble() %>% 
  select(iso3c = CountryCode) %>% 
  mutate(value = 1) %>% 
  convertGDP(unit_in  = 'constant 2005 US$MER',
             unit_out = 'constant 2017 US$MER',
             replace_NAs = 'with_USA') %>% 
  rename(convertGDP_with_replace = 'value') %>%
  mutate(value = 1) %>% 
  convertGDP(unit_in  = 'constant 2005 US$MER',
             unit_out = 'constant 2017 US$MER') %>% 
  rename(convertGDP_without_replace = 'value') %>% 
  arrange(convertGDP_with_replace != convertGDP_without_replace)
# # A tibble: 249 × 3
#    iso3c convertGDP_with_replace convertGDP_without_replace
#    <chr>                   <dbl>                      <dbl>
#  1 USA                      1.23                       1.23
#  2 ABW                      1.23                       1.24
#  3 AFG                      1.23                       1.34
#  4 AGO                      1.23                       1.10
#  5 ALB                      1.23                       1.07
#  6 AND                      1.23                       1.03
#  7 ARE                      1.23                       1.43
#  8 ARG                      1.23                       2.35
#  9 ARM                      1.23                       1.46
# 10 ASM                      1.23                       1.50
# # ℹ 239 more rows
# # ℹ Use `print(n = ...)` to see more rows
# Warning message:
# NAs have been generated for countries lacking conversion factors! 

Clarification is very much appreciated.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q 0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q added the bug Something isn't working label Oct 10, 2024
@johanneskoch94 johanneskoch94 transferred this issue from pik-piam/mrdrivers Oct 10, 2024
@johanneskoch94
Copy link
Member

johanneskoch94 commented Oct 10, 2024

Oh my... I apologize for this, and thanks for raising the issue. This looks very bad, but I'm 80% certain, that the calculation is actually what is intended. So the documentation is wrong (and the actual implementation also completely misleading). I will check this with @fbenke-pik to be certain, and work on a fix straight away.

(It stems from a sloppy implementation of "with_USA" (related to #30), and the argument "use_USA_deflator_fall_all"...)

Edit: actually, I think this is very bad, and not what was intended. The default used to be replace_NAs = "linear", or some combination, such as replace_NAs = c("linear", "no_conversion"). Then the option "with_USA" was introduced, to be used in combination with "linear", c("linear", "with_USA"). But then @fbenke-pik found #30, and after checking with me (and me not realizing there was a bug), switched to using only "with_USA". So I assume, that you, @fbenke-pik, initially wanted to use c("linear", "with_USA"), so not overwrite all conversion factors with the US ones, but only missing ones, right?

@fbenke-pik
Copy link

This is not what I intended, I was expecting to only use USA conversion factors in cases where no country-specific conversion factor was found.

Johannes, can you provide a quick bugfix, or should we switch to another fallback option in replace_NAs?

@fbenke-pik
Copy link

Thanks for spotting this, Michaja!

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

I am going to miss the "is chemicals production in 2070 in Uzbekistan 1.23 times of what it used to be" metric while debugging 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants