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

Oil refineries ng processing #51

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

haclohman
Copy link
Collaborator

ng_processing_proxy and oil_refineries_proxy

Creation of the ng_processing_proxy and oil_refineries_proxy
Added uniform distribution of emissions across state for states missing proxy data.
Changed Header to be consistent with other proxy headers.

Removed imports that were unused (please verify with author that these were not used):
- os
- pyarrow.parquet
- V3_DATA_PATH

Added function docstring with Args

Questions:
- Are lines 388 and 394 redundant? It may not impact the script performance, but I wanted to check.

- Lines 441, 442. Should line 441 be commented out as well if assert is commented out on line 442, as sums may not need to be calculated if it is not being asserted.

Ignored lines >88 char due to object name lengths and process complexity.

Added some documentation context, but overall code was commented well.
Changed Header to make consistent with other proxy scripts.

Removed parquet import, unused in script.

Added pytask function docstring.

Minor edits to bring some lines <88 char.

Added some documentation in larger chunks.

Overall well documented.
@burnettear
Copy link
Collaborator

@haclohman I committed updates for task_ng_processing_proxy and task_oi_refineries_proxy. Overall, scripts were well documented. I added notes to commit edits. Primary edits were to header and chunk documentation to make similar to other proxy scripts.

Let me know if you approve the edits or want to follow up. Then we can ping Nick to review/merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants