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

Rename some straggler assets #3294

Merged
merged 11 commits into from
Jan 31, 2024
Merged

Rename some straggler assets #3294

merged 11 commits into from
Jan 31, 2024

Conversation

bendnorman
Copy link
Member

@bendnorman bendnorman commented Jan 25, 2024

Overview

This PR renames some straggler assets we missed in #2818. I double checked all assets follow the new naming convention.

The only remaining task is to figure out if we want to use a word or phrase other than exploded.

Testing

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

To-do list

Preview Give feedback

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bendnorman bendnorman requested a review from cmgosnell January 25, 2024 01:10
@bendnorman bendnorman linked an issue Jan 25, 2024 that may be closed by this pull request
@bendnorman bendnorman self-assigned this Jan 25, 2024
@bendnorman bendnorman added the dagster Issues related to our use of the Dagster orchestrator label Jan 25, 2024
Copy link
Member

@cmgosnell cmgosnell left a comment

Choose a reason for hiding this comment

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

left a few questions and suggestions but overall this is looking good!

src/pudl/convert/censusdp1tract_to_sqlite.py Outdated Show resolved Hide resolved
src/pudl/extract/eia860.py Show resolved Hide resolved
src/pudl/extract/ferc1.py Show resolved Hide resolved
src/pudl/output/ferc1.py Outdated Show resolved Hide resolved
@bendnorman bendnorman requested a review from cmgosnell January 25, 2024 20:44
Copy link
Member

@cmgosnell cmgosnell left a comment

Choose a reason for hiding this comment

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

new names who this?! lol this looks great :-)

@bendnorman bendnorman marked this pull request as ready for review January 26, 2024 18:51
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0d6713) 92.69% compared to head (ec473bc) 92.70%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3294      +/-   ##
==========================================
+ Coverage   92.69%   92.70%   +0.01%     
==========================================
  Files         145      145              
  Lines       13100    13116      +16     
==========================================
+ Hits        12142    12158      +16     
  Misses        958      958              

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

@bendnorman bendnorman enabled auto-merge (squash) January 26, 2024 22:46
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.

😬 sorry

src/pudl/output/pudltabl.py Outdated Show resolved Hide resolved
src/pudl/analysis/state_demand.py Outdated Show resolved Hide resolved
src/pudl/output/ferc1.py Outdated Show resolved Hide resolved
src/pudl/transform/ferc1.py Outdated Show resolved Hide resolved
src/pudl/convert/censusdp1tract_to_sqlite.py Outdated Show resolved Hide resolved
src/pudl/output/eia_bulk_elec.py Outdated Show resolved Hide resolved
src/pudl/transform/ferc714.py Outdated Show resolved Hide resolved
src/pudl/output/censusdp1tract.py Outdated Show resolved Hide resolved
@zaneselvans zaneselvans disabled auto-merge January 27, 2024 02:03
@zaneselvans zaneselvans added this to the v2024.01 milestone Jan 29, 2024
@bendnorman bendnorman marked this pull request as draft January 29, 2024 22:19
@bendnorman
Copy link
Member Author

The latest commit:

  • renames core_ferc714__hourly_demand_pa to out_ferc714__hourly_estimated_state_demand to be consistent with out_ferc714__hourly_planning_area_demand
  • renames _core_xbrl_ferc1__calculation_components to _core_ferc1_xbrl__calculation_components
  • renames _out_eia__monthly_state_average_fuel_costs to _out_eia__monthly_state_average_fuel_prices
  • renames raw_census__demographic_profiles_dp1 to raw_censusdp1tract__all_tables
  • makes the census assets intermediate assets

There are a couple of remaining issues:

@zaneselvans
Copy link
Member

Ah okay I see what you're saying about the _entity_ now. To me these geospatial entities feel pretty different from the other kinds of things that we've called entities, which I think are: utilities, plants, generators, boilers, and balancing authorities. In a strict sense I could see the argument, but (as with Pluto) I think if we were to call states, counties, and census tracts entities, then there are a whole lot of other things we would end up categorizing that way too, so I'd be inclined to keep them out of the entity club, and think together a little bit about what "entity" means in the context of PUDL.

Thus far I think the concept has focused on energy-system specific entities that typically show up in a number of different contexts, often in association with a lot of denormalized data that isn't necessarily internally consistent, and the "entity" that we extract is our best guess at the correct canonical values that should be associated with the entity.

Other "entities" I could imagine us introducing someday might include: smokestacks (emissions units), water cooling systems, natural gas pipeline compressors, LNG liquefaction facilities, or underground storage sites from PHMSA, coal mines from MSHA, electric substations, ISOs/RTOs, NERC regions, etc.

I'll respond over in the alembic discussion.

@bendnorman
Copy link
Member Author

That's fair! If calling census areas entities would greatly expand the definition of what an entity is PUDL, then I'm down to restrict its use to just energy system topics: smoke stacks, substations, plants, generators...

I'll remove entity from the census assets and recreate the migrations.

@bendnorman
Copy link
Member Author

Ok I removed "entity" from the census assets and recreated the migrations. I also made the census assets plural.

@zaneselvans zaneselvans marked this pull request as ready for review January 30, 2024 23:05
@bendnorman bendnorman enabled auto-merge (squash) January 30, 2024 23:06
@bendnorman bendnorman merged commit ab36601 into main Jan 31, 2024
18 checks passed
@bendnorman bendnorman deleted the rename-remaining-assets branch January 31, 2024 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dagster Issues related to our use of the Dagster orchestrator
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Apply naming convention to straggler assets
3 participants