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

standardize the calc checks for the total to subtotal calcs #2886

Merged
merged 3 commits into from
Oct 6, 2023

Conversation

cmgosnell
Copy link
Member

PR Overview

I don't love this overall... but it is a first attempt to standardize the way we are checking the total to subtotal calcs. I:

  • added a is_total_to_subdimensions_calc bool column back into the calc components table
  • in both the transform step (in reconcile_table_calculations) and in the explode step (in reconcile_intertable_calculations) we now use calculate_values_from_components and check_calculation_metrics twice: once for the "normal" calcs and once for the total-to-subdimension calcs.

Notes:

  • this did not fix the high error rate on the balance sheet asset explosion.
  • I still think it feels nicer overall bc is removes almost duplicate code for the old version of this check.
  • I think it could be a cleaner if we thought through a way to just have these two calc-ing/metrics-ing functions deal with the logic of what is a total-to-sub-dim calc instead of calling them twice. I tried adding the is_total_to_subdimensions_calc into the pks of the groupby and ran into a slight snag.. i still think its possible but i wanted to get a proof out before leaving.
  • Because this didn't solve the asset explosions' problem... my suspicion is this is because this electric_plant_depreciation_change_ferc1 has... ALLL OF THE DIMENSIONS. and perhaps we are introducing some double counting into the calc components because of it? not sure at all just a hunch.

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.

@zaneselvans zaneselvans linked an issue Sep 25, 2023 that may be closed by this pull request
Base automatically changed from explode_calc_standardization to dev September 25, 2023 23:11
@cmgosnell cmgosnell marked this pull request as ready for review October 6, 2023 18:19
@cmgosnell cmgosnell requested review from zaneselvans and removed request for e-belfer October 6, 2023 18:19
Comment on lines +1488 to +1503
logger.info("Checking sub-total calcs.")
subtotal_calcs = pudl.transform.ferc1.calculate_values_from_components(
calculation_components=calculations_intertable[
calculations_intertable.is_total_to_subdimensions_calc
],
data=exploded,
calc_idx=calc_idx,
value_col=self.value_col,
)
subtotal_calcs = pudl.transform.ferc1.check_calculation_metrics(
calculated_df=subtotal_calcs,
value_col=self.value_col,
calculation_tolerance=self.calculation_tolerance.intertable_calculation_errors,
table_name=self.root_table,
add_corrections=True,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

there are no total to sub-dimension inter-table calcs rn but this felt like a good thing to run nonetheless

@zaneselvans zaneselvans added ferc1 Anything having to do with FERC Form 1 xbrl Related to the FERC XBRL transition labels Oct 6, 2023
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (67822df) 88.6% compared to head (60b13c2) 88.6%.
Report is 1 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##             dev   #2886     +/-   ##
=======================================
- Coverage   88.6%   88.6%   -0.1%     
=======================================
  Files         90      90             
  Lines      10809   10808      -1     
=======================================
- Hits        9577    9576      -1     
  Misses      1232    1232             
Files Coverage Δ
src/pudl/output/ferc1.py 88.8% <100.0%> (+<0.1%) ⬆️
src/pudl/transform/ferc1.py 96.6% <100.0%> (-0.1%) ⬇️

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

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.

Definitely an improvement!

@zaneselvans zaneselvans merged commit 8315219 into dev Oct 6, 2023
11 checks passed
@cmgosnell cmgosnell deleted the explode_standardize_total_to_subdim_calc_checks branch October 18, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ferc1 Anything having to do with FERC Form 1 xbrl Related to the FERC XBRL transition
Projects
Archived in project
2 participants