-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
Integrate income_statement_ferc1
table
#2147
Conversation
Codecov ReportBase: 85.3% // Head: 85.4% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## dev #2147 +/- ##
=====================================
Coverage 85.3% 85.4%
=====================================
Files 73 73
Lines 8746 8777 +31
=====================================
+ Hits 7469 7496 +27
- Misses 1277 1281 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I guess we never figured out why you were having weird Numpy failures in the CI. If the builds pass would you bump the max Numpy version back up to |
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.
Hey these changes look good. Hopefully the numpy thing isn't an issue!
Overview
Closes #1813
This tbl is weird because it has two dbf tabels flowing into one pudl table.
utility_type
while the xbrl has multiple columns for eachincome_type
align_row_numbers_dbf
to take a list ofdbf_table_names
instead of a singledbf_table_name
. This felt a little silly because this many be only one table that ever needs this but it was very easy to implement and felt simpler than having two version ofalign_row_numbers_dbf
source_table_id
because theFerc1AbstractTableTransformer
assumes there is only one source table. The new version uses the table name that was added during the extract step. We cooooould do this for all of the DBF tables, but we would need to always add the XBRL table name directly from the extract step. That all felt too.. much for one table. The main quirk here is thatsource_table_id
in the mainassign_record_id
method takes asource_ferc1
and adf
. for all of the other tables the df does nada, but the income statement table needs it. The default method as kwargs so this works okay but feels a little weird.PR Checklist
Before requesting a review of your pull request, please make sure you've done the
following:
dev
(or the appropriate upstream branch) intoyour branch and resolved any merge conflicts. You may need to do this several
times over the course of a PR as
dev
changes frequently.Running Tests with Tox
for details on how to run the full test suite locally if you need to debug a
particular failure.
descriptive enough for developers and users to understand your code.
data validation tests
pass locally on a fresh DB.
unit tests.
will catch unexpected data issues.
release notes
to reflect your changes. Make sure to reference the PR and any related issues.
questions you'd like reviewers to answer, known issues, solutions you're
unsatisfied with, or other things that deserve special attention from the
reviewer.