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 xbrl metadata renaming in the electric_plant_depreciation_functional_ferc1 table #2712

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

cmgosnell
Copy link
Member

@cmgosnell cmgosnell commented Jul 5, 2023

PR Overview

mkay i did some more upstream cleaning of the xbrl_factoid names in this table. this was an error bc the columns actually coming from the xbrl data did not line up with the columns in the xbrl metadata so our standing mapping of xbrl name to pudl name was not working.

PR Checklist

  • Merge the most recent version of the branch you are merging into (probably dev).
  • All CI checks are passing. Run tests locally to debug failures
  • Make sure you've included good docstrings.
  • For major data coverage & analysis changes, run data validation tests
  • Include unit tests for new functions and classes.
  • Defensive data quality/sanity checks in analyses & data processing functions.
  • Update the release notes and reference reference the PR and related issues.
  • Do your own explanatory review of the PR to help the reviewer understand what's going on and identify issues preemptively.

Copy link
Member

@zaneselvans zaneselvans left a comment

Choose a reason for hiding this comment

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

So this is a naming inconsistency that we just hadn't previously identified, and you moved the special case out of process_xbrl_metadata() and into the actual renaming function and made it account for the newly identified inconsistency fully?

@cmgosnell
Copy link
Member Author

So this is a naming inconsistency that we just hadn't previously identified, and you moved the special case out of process_xbrl_metadata() and into the actual renaming function and made it account for the newly identified inconsistency fully?

Yup! the only factoid w/ a calc was the total and basically everything else was not effectively mapped in our old version. applying the rename in this new way does it earlier in the process and enabled me to remove the bespoke one-factoid rename.

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch coverage: 100.0% and no project coverage change.

Comparison is base (048990e) 88.5% compared to head (9479830) 88.6%.

Additional details and impacted files
@@              Coverage Diff              @@
##           explode_ferc1   #2712   +/-   ##
=============================================
  Coverage           88.5%   88.6%           
=============================================
  Files                 87      87           
  Lines              10289   10289           
=============================================
+ Hits                9115    9117    +2     
+ Misses              1174    1172    -2     
Impacted Files Coverage Δ
src/pudl/transform/ferc1.py 96.8% <100.0%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zaneselvans zaneselvans merged commit 9a960cc into explode_ferc1 Jul 5, 2023
@zaneselvans zaneselvans deleted the explode_deprish_meta_fix branch July 5, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants