-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
add minor inter table calc fixes #2635
add minor inter table calc fixes #2635
Conversation
src/pudl/transform/ferc1.py
Outdated
# these next two fixes are related. we're replacing | ||
# commercial_and_industrial_sales which is from a different table | ||
# with two swappable components from this table. There is no way rn | ||
# to say swap thing A for thing B & C for two so we swap A for B and | ||
# then add C. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this comment is saying. What are "two swappable components?"
We're deleting one component and adding two components, right?
It seems like it might be simpler to have the more atomic "addition" and "deletion" operations rather than always defaulting to "replace" which is a composite of a deletion and an addition. We could define a replacement method in terms of deletion and addition methods.
What happens when you replace a component that has a list of source tables with a component that doesn't have a list of source tables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what's happening is
# Replace commercial_and_industrial_sales which is reported in
# electricity_sales_by_rate_schedule_ferc1 with two components
# (small_or_commercial_sales... and large_or_industrial_sales...)
# from this table which should sum to the same amount as the value
# being replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code wise i hard agree that having "additions" and "deletions" would make way more sense. As a human conceptually it makes way more sense to say "replace A for B" (and A or B can be a null element which results in "add B" or "delete A" respective).
i only added that comment bc rn the fixes are setup as "replace A for B" not "replace A for B & C" which is what needed to happen here. but that needed to be added as "replace A for B" & "add C".
i'd love a deeper structural change for these calcs but that is for #2605.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## xbrl_meta_reshape #2635 +/- ##
=================================================
Coverage 87.1% 87.1%
=================================================
Files 84 84
Lines 9903 9912 +9
=================================================
+ Hits 8627 8636 +9
Misses 1276 1276
☔ View full report in Codecov by Sentry. |
PR Overview
small pr to address some suggestions from #2622
additional changes this provoked:
source_table
->source_tables
(bc it is a list of tables - usually a list of 1 but still)xbrl_factoid
out ofprocess_xbrl_metadata
into it own method. this is by default just a check/a no-op, but now three table transformers have bespoke version. This was necessary because we added a calc to fix to a table that had duplicative metadata, which broke the calc fixer bc it expects to be able to extract a calculation from the metadata based on anxbrl_factoid
name. So now the deduping of the metadata happens withinprocess_xbrl_metadata
but beforeapply_xbrl_calculation_fixes
PR Checklist
dev
).