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

squelch countrycode warnings when no currency is matched #28

Merged
merged 2 commits into from
Jun 6, 2024

Conversation

cjyetman
Copy link
Member

in particular, when the country code is PS and there's no matching currency code

countrycode::countrycode("PS", "iso2c", "iso4217c")
#> Warning: Some values were not matched unambiguously: PS
#> [1] NA
countrycode::countrycode("PS", "iso2c", "iso4217c", warn = FALSE)
#> [1] NA

⚠️ I'm not 100% sure it's appropriate to ignore all/any warning here

Copy link

codecov bot commented May 28, 2024

Codecov Report

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

Project coverage is 18.82%. Comparing base (a37a7d2) to head (f3be78a).

Files Patch % Lines
.../prepare_iss_average_sector_emission_intensities.R 0.00% 1 Missing ⚠️
R/prepare_iss_entity_emission_intensities.R 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #28   +/-   ##
=======================================
  Coverage   18.82%   18.82%           
=======================================
  Files          35       35           
  Lines        1126     1126           
=======================================
  Hits          212      212           
  Misses        914      914           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jdhoffa jdhoffa left a comment

Choose a reason for hiding this comment

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

@cjyetman I trust your judgement if you think this is a reasonable thing to do.

Copy link

@AlexAxthelm AlexAxthelm left a comment

Choose a reason for hiding this comment

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

LGTM. Should we be filtering to the countrycode list in workflow.factset?

@cjyetman
Copy link
Member Author

I think maybe we should filter rows where a currency cannot be determined after this, but have to think about that when I'm not in a brain fog

@cjyetman
Copy link
Member Author

putting in draft until I determine if it's necessary/appropriate to filter out rows that end up with currency code == NA after this

@cjyetman
Copy link
Member Author

cjyetman commented Jun 5, 2024

These are filters 🤣. I was thinking these are adding the currency based on the country code, but the currency is already there. This is making sure that we're only keeping rows where the currency matches the country (I think so we drop rows of data for foreign versions of the same thing?).

But anyway, I don't think there's much risk with this. {countrycode} may throw warnings if

  1. the country code is not known/invalid
  2. there is no known currency code for the given country (this is the PS case)

but either of those warnings are not very important, because those rows where the country/currency pair cannot be verified/validated will be filtered out, which is what we want.

It's a bit of a shame that we therefore cannot verify any PS rows because PS does not have an official currency recognized in iso4217c, but these warnings will not help get around that.

opening for review

@cjyetman cjyetman marked this pull request as ready for review June 5, 2024 22:40
Copy link

@AlexAxthelm AlexAxthelm left a comment

Choose a reason for hiding this comment

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

Thanks for investigating. Looks good.

@cjyetman cjyetman merged commit 6cf423a into main Jun 6, 2024
9 checks passed
@cjyetman cjyetman deleted the squelch-countrycode-warnings branch June 6, 2024 09:52
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