-
-
Notifications
You must be signed in to change notification settings - Fork 128
Migrate eia930 metadata #4420
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
base: main
Are you sure you want to change the base?
Migrate eia930 metadata #4420
Conversation
0138cc7
to
a3cb7ef
Compare
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.
Blocking: one date change we should make sure is intentional, but I trust you and don't need to review again to approve.
Remaining comments are nonblocking.
There are three experimental tables in this PR alone; is that enough to add a built-in warning to use in all three places? Could be a quick/easy followup.
In theory, the sum of net generation across all energy sources should equal the total net generation reported in the balancing authority operations table. In practice, | ||
there are many cases in which these values diverge significantly, which require further investigation.""", |
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'd throw in a known_discrepancies
Usage Warning for this to make it more likely people will look for and see this text
"""EXPERIMENTAL / WORK-IN-PROGRESS, 2025-04-04. | ||
"description": { | ||
"additional_summary_text": "balancing authority net generation, interchange, and demand with imputed demand.", | ||
"additional_details_text": """(EXPERIMENTAL / WORK-IN-PROGRESS, 2025-03-31) |
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.
Date change on purpose or accident?
vetted yet.""" | ||
), | ||
This table is based on ``core_eia930__hourly_operations``, but adds imputed demand where the original data was missing or anomalous. Codes explaining why values have been imputed can be found in the ``core_pudl__codes_imputation_reasons`` table.""", | ||
"usage_warnings": ["imputed_values"], |
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.
Good use of imputed_values
here.
This table is available in the nightly builds during development, but has not been fully | ||
vetted yet.""" |
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 feel great about removing this text. Is the idea that it's already covered by the EXPERIMENTAL header on additional details?
Overview
What problem does this address?
Migrates the metadata for EIA930 tables into the new metadata format
What did you change?
table description metadata in
pudl/metadata/resources/eia930.py
Documentation
Make sure to update relevant aspects of the documentation:
docs/data_sources/templates
).src/metadata
).Testing
How did you make sure this worked? How can a reviewer verify this?
To-do list
dbt
tests.make pytest-coverage
locally to ensure that the merge queue will accept your PR.make pytest-coverage
passes, make sure you have a fresh full PUDL DB downloaded locally, materialize new/changed assets and all their downstream assets and run relevant data validation tests usingpytest
and--live-dbs
.make pytest-validate
.build-deploy-pudl
GitHub Action manually.