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

Handle WP NEM12 edge-case with missing MSATSLoadDateTime col #48

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

atyndall
Copy link
Contributor

@atyndall atyndall commented Dec 7, 2023

Western Power NEM12 files don't always include an empty column for MSATSLoadDateTime if it is never present, instead the column can be completely absent.

This occurs with files downloaded from their internal portal and causes a ValueError exception to be unnecessarily raised on parse, as the number of columns is less than expected.

The AEMO Meter Data File Format Specification NEM12 & NEM13 states that MSATSLoadDateTime is required if available, but is not mandatory.

As such, instead of erroring when the final MSATSLoadDateTime column isn't present, we can proceed and fill it with None with no consequences.

Validation for parse_300_row has been modified to ensure that the error is only raised if number of required non-reading fields is less than 6, instead of equals 7.

These WP NEM12 files otherwise seem to parse fine with this change.

@pep8speaks
Copy link

pep8speaks commented Dec 7, 2023

Hello @atyndall! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-12-07 06:51:33 UTC

Western Power NEM12 files don't always include an empty column for MSATSLoadDateTime if it is never present, instead the column can be completely absent.
This occurs with files downloaded from their internal portal and causes a ValueError exception to be unnecessarily raised on parse, as the number of columns is less than expected.

The AEMO "Meter Data File Format Specification NEM12 & NEM13" states that MSATSLoadDateTime is required if available, but is not mandatory.
As such, instead of erroring when the final MSATSLoadDateTime column isn't present, we can proceed and fill it with None with no consequences.
Validation for parse_300_row has been modified to ensure that the error is only raised if number of required non-reading fields is less than 6, instead of equals 7.
These WP NEM12 files otherwise seem to parse fine with this change.
@atyndall atyndall force-pushed the western-power-fixes branch from 1350d40 to 85c4983 Compare December 7, 2023 06:51
@atyndall atyndall changed the title Handle Western Power NEM12 edge-case with missing MSATSLoadDateTime column Handle WP NEM12 edge-case with missing MSATSLoadDateTime col Dec 7, 2023
@aguinane
Copy link
Owner

aguinane commented Dec 8, 2023

Hi @atyndall - it looks like one of the tests that @matt-telstra added for his use case is failing as a result of the change.
@matt-telstra you mentioned in #40 that the 7 number should probably change so that could be the issue?
@atyndall - can you provide a simple example file for the tests of a valid western power file with the issue as well? (change the NMI etc if it's a real example and remove unneeded rows).

@atyndall
Copy link
Contributor Author

Hi @aguinane, attached is a sample that works with my changes, but doesn't work with the currently released version.

WP_Sample.csv

@aguinane aguinane merged commit f3afabb into aguinane:master Dec 14, 2023
0 of 4 checks passed
@aguinane
Copy link
Owner

@matt-telstra I've merged this one in as I'd rather fall on the side of not raising errors then raising if in conflict (can use the strict flag otherwise). 2/3 of your tests still passed but one failed. If you think this test should throw an error still might be worth making use of the strict flag?

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