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

Update FERC rate base tags with RMI guidance #3162

Merged
merged 9 commits into from
Dec 23, 2023
Merged

Conversation

cmgosnell
Copy link
Member

@cmgosnell cmgosnell commented Dec 15, 2023

Overview

Closes #2600 .

What problem does this address?
This pr integrates updates from the RMI review:

Tasks

Preview Give feedback

What did you change?

Testing

How did you make sure this worked? How can a reviewer verify this?

To-do list

Preview Give feedback

@cmgosnell cmgosnell self-assigned this Dec 15, 2023
this results in a messy calculation table bc rn we generally expect that
the parent and child fact have the same dimensions (with the expection of
the subtotal calculations). so imma delete this and migrate this change to
and aggregatable tag. but wanted to save just in case.
@cmgosnell cmgosnell marked this pull request as ready for review December 22, 2023 17:17
Comment on lines -1158 to -1160
# Also, these tags may not be applicable to all exploded tables, but
# we need to pass in a dataframe with the right structure to all of the exploders,
# so we're just re-using this one for the moment.
Copy link
Member Author

Choose a reason for hiding this comment

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

now all the explosions have tags so bye bye note

# we need to pass in a dataframe with the right structure to all of the exploders,
# so we're just re-using this one for the moment.
rate_base_tags = _rate_base_tags(table_dimensions_ferc1=table_dimensions_ferc1)
rate_tags = _get_tags("xbrl_factoid_rate_base_tags.csv", table_dimensions_ferc1)
Copy link
Member Author

Choose a reason for hiding this comment

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

i decided to standardize the tags bc now there are a million so its either _get_tags which just grabs a csv and fills in the dimensions or the per-existing _aggregatable_dimension_tags which is now used for all three of our dimensions.

then instead of a bunch of little merges i used concat to squish em all together.

Copy link
Member Author

@cmgosnell cmgosnell Dec 22, 2023

Choose a reason for hiding this comment

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

its hard to see the changes here because all of the lines changed bc the table_name's changed in the great rename. but the main this to know about all of these changes is that a) they came straight from rmi jon and their utility account expert's expert and b) they only initially caused one error with the explosions that i was very easily able to fix by adding some specified dimensions into the tags (i'll point that out in that csv)

Comment on lines +162 to +166
core_ferc1__yearly_utility_plant_summary_sched200,depreciation_utility_plant_in_service,other,,,net_utility_plant
core_ferc1__yearly_utility_plant_summary_sched200,depreciation_utility_plant_in_service,gas,,,net_utility_plant
core_ferc1__yearly_utility_plant_summary_sched200,depreciation_utility_plant_in_service,common,,,net_utility_plant
core_ferc1__yearly_utility_plant_summary_sched200,depreciation_utility_plant_in_service,other3,,,net_utility_plant
core_ferc1__yearly_utility_plant_summary_sched200,depreciation_utility_plant_in_service,other2,,,net_utility_plant
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only tag from @jrea-rmi that i needed to edit a little bit. the utility_type=="electric" version of this factoid is linked to other more granular facts that have their own rate_base_category that are more specific than net_utility_plant. This was causing the error Calculation forest nodes have conflicting tags: so i removed the generic category and labeled only the non-electric utility types.

Copy link
Member Author

Choose a reason for hiding this comment

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

i tried to add these guys into the transform but it broke our convention of the calculations having the same dimensions for the parent and child facts (other than the total-to-subtotal calcs). I could probably figure out how to integrate this into the calc components but this felt much simpler and better overall.

@cmgosnell cmgosnell requested a review from e-belfer December 22, 2023 17:57
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (539a6f1) 92.6% compared to head (4f85ab8) 92.6%.
Report is 16 commits behind head on dev.

Additional details and impacted files
@@          Coverage Diff          @@
##             dev   #3162   +/-   ##
=====================================
  Coverage   92.6%   92.6%           
=====================================
  Files        134     134           
  Lines      12611   12615    +4     
=====================================
+ Hits       11682   11686    +4     
  Misses       929     929           

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

Copy link
Member

@e-belfer e-belfer left a comment

Choose a reason for hiding this comment

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

I have a clarifying question about the table names getting used here. Otherwise this looks straightforward and should be good to go, if it's working as expected on your end!

electric_operating_expenses_ferc1,customer_service_and_information_expenses,
electric_operating_expenses_ferc1,sales_expenses,
electric_operating_expenses_ferc1,administrative_and_general_expenses,general
core_ferc1__yearly_plant_in_service_sched204,intangible_plant,intangible
Copy link
Member

Choose a reason for hiding this comment

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

Since we're using the table names with the schedules here (which differ from the table names in PUDL), is there a translation step I'm not seeing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

we ended up landing on table names with the schedules in them!

Copy link
Member Author

Choose a reason for hiding this comment

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

its a bit of a divergence from the other tables but it ended up feeling like the easiest way to communicate to users which DBF or XBRL table the data originated from.

Copy link
Member

Choose a reason for hiding this comment

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

ah then I'm hopelessly out of date post rename!

@e-belfer e-belfer self-requested a review December 22, 2023 21:30
Copy link
Member

@e-belfer e-belfer left a comment

Choose a reason for hiding this comment

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

Looks good to merge!

@cmgosnell cmgosnell enabled auto-merge December 22, 2023 21:31
@cmgosnell cmgosnell merged commit 9fcb79a into dev Dec 23, 2023
13 checks passed
@cmgosnell cmgosnell deleted the explode_tag_updates branch December 23, 2023 09:38
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