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.

Sign up for GitHub

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

Jump to bottom

Make core ferc1 asset names plural and rename exploded ferc1 assets #2914

Closed
wants to merge 2 commits into from
Closed

Make core ferc1 asset names plural and rename exploded ferc1 assets #2914

wants to merge 2 commits into from

Conversation

bendnorman
Copy link
Member

PR Overview

This PR renames a few core ferc1 asset names to be plural (core_ferc1__yearly_plants_hydro -> core_ferc1__yearly_hydro_plants) plural and renames the exploded ferc1 output assets.

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.

@codecov
Copy link

codecov bot commented Oct 3, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (791a70b) 88.5% compared to head (4260c2e) 88.5%.
Report is 1 commits behind head on rename-mcoe-assets.

Additional details and impacted files 
@@                Coverage Diff                 @@
##           rename-mcoe-assets   #2914   +/-   ##
==================================================
  Coverage                88.5%   88.5%           
==================================================
  Files                      90      90           
  Lines                   10819   10821    +2     
==================================================
+ Hits                     9580    9582    +2     
  Misses                   1239    1239
Files Coverage Δ
src/pudl/analysis/classify_plants_ferc1.py 92.4% <ø> (ø)
src/pudl/analysis/ferc1_eia.py 98.0% <ø> (ø)
src/pudl/analysis/ferc1_eia_train.py 52.9% <ø> (ø)
src/pudl/analysis/mcoe.py 97.4% <100.0%> (ø)
src/pudl/analysis/plant_parts_eia.py 96.5% <ø> (ø)
src/pudl/extract/ferc1.py 99.1% <ø> (ø)
src/pudl/glue/ferc1_eia.py 98.5% <ø> (ø)
src/pudl/metadata/fields.py 100.0% <ø> (ø)
src/pudl/metadata/resources/eia.py 100.0% <ø> (ø)
src/pudl/metadata/resources/ferc1.py 100.0% <ø> (ø)
... and 5 more

... and 1 file with indirect coverage changes

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

@bendnorman bendnorman requested a review from e-belfer October 3, 2023 18:37
e-belfer
e-belfer requested changes Oct 4, 2023
View reviewed changes
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.

Pluralizing is fine, but I'm not 100% following the logic of which assets are getting pluralized - seems like there are at least a few that haven't been but could logically also be (e.g. summary vs summaries, cash flow vs cash flows).

There are a few other assets (e.g. calculation_components_xbrl_ferc1, table_dimensions_xbrl_ferc1) added by the explosion process in pudl.transform.ferc1 and pudl.output.ferc1 that should also get renamed, probably in this PR?

Otherwise fast ETL runs as expected.

docs/release_notes.rst
@@ -223,7 +224,7 @@ Data Cleaning
* Fixed column naming issues in the
:ref:`core_ferc1__yearly_electric_operating_revenues` table.
* Made minor calculation fixes in the metadata for
:ref:`core_ferc1__yearly_income_statement`,
:ref:`core_ferc1__yearly_income_statements`,
:ref:`core_ferc1__yearly_utility_plant_summary`,
Copy link
Member

Choose a reason for hiding this comment

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

plant_summaries by this logic, or no?

docs/release_notes.rst
@@ -97,7 +97,8 @@ Data Coverage
:pr:`2134`.
* :ref:`core_ferc1__yearly_depreciation_amortization_summary`, see issue :issue:`1816`
Copy link
Member

Choose a reason for hiding this comment

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

depreciation_amortization_summaries?

src/pudl/output/ferc1.py
@@ -1037,7 +1037,10 @@ def exploded_table_asset_factory(
}
ins |= {table_name: AssetIn(table_name) for table_name in table_names_to_explode}

@asset(name=f"exploded_{root_table}", ins=ins, io_manager_key=io_manager_key)
asset_group, asset_name = root_table.split("__")
asset_name = f"{asset_group}__exploded_{asset_name}"
Copy link
Member

Choose a reason for hiding this comment

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

Is exploded the asset type in this model? Seems like something might be missing here, possibly yearly?

@bendnorman
Rename heat_rate_mmbtu_mwh -> heat_rate_mmbtu_mwh_by_unit (#2917)
4260c2e 
* Rename heat_rate_mmbtu_mwh -> heat_rate_mmbtu_mwh_by_unit

* Rename heat rate mmbtu mwh to follow existing naming convention
Base automatically changed from rename-mcoe-assets to rename-core-assets October 6, 2023 19:58
@bendnorman bendnorman mentioned this pull request Oct 31, 2023
Closed
@bendnorman
Copy link
Member Author

Closed in favor of #2995

@bendnorman bendnorman closed this Nov 2, 2023
@bendnorman bendnorman mentioned this pull request Nov 16, 2023
Closed
@jdangerx jdangerx deleted the rename-ferc1-core-assets branch January 2, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e-belfer e-belfer e-belfer requested changes

Assignees

@bendnorman bendnorman

Labels
None yet
Projects
Catalyst Megaproject
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bendnorman @e-belfer