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

Refine fill forward strategy to handle issue with summary table #24

Merged
merged 9 commits into from
Sep 4, 2024

Conversation

prakaa
Copy link
Member

@prakaa prakaa commented Sep 3, 2024

Previously, the fill forward strategy for multi-header rows meant that single header columns after multi-headers would be mislabelled (see issue below for an example). This seems to only occur in the summary tables

To fix this, fill forward is manually implemented to:

  1. Accept the intermediate header row and the preceding header row
  2. If a value in the intermediate header row is nan, then:
    a) If value in corresponding column of the preceding header row is nan, then forward fill
    b) else leave as nan

This should work for 3+ header rows, but AEMO only seems to have at most 3 header rows at the moment

Ran all tests and produced all output - note that all changes are only new separator (this is branched off that PR) and removing wrong merged headers in summary files. So for easiest comparison, diff these changes with those of the new separator branch

Closes #23

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/isp_workbook_parser/read_table.py 90.90% 0 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
src/isp_workbook_parser/config_model.py 100.00% <ø> (ø)
src/isp_workbook_parser/parser.py 84.66% <100.00%> (+4.00%) ⬆️
src/isp_workbook_parser/read_table.py 98.14% <90.90%> (-1.86%) ⬇️

Copy link
Member

@nick-gorman nick-gorman left a comment

Choose a reason for hiding this comment

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

I've inspected how the functionality works on a couple of tables in summary mapping and it appears to be working properly.

@nick-gorman nick-gorman merged commit 1438be6 into main Sep 4, 2024
15 checks passed
@nick-gorman nick-gorman deleted the refine-fill-forward-strategy branch September 4, 2024 00:49
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.

read_table issue with summary tables
2 participants