-
Notifications
You must be signed in to change notification settings - Fork 5
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
NPI-3064 Fix SP3 output formatting of nodata values, to avoid writing NaNs to SP3 file #12
Conversation
…nodata values, to avoid outputting 'NaN' in output files (not in line with SP3 spec). Fix missing import.
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.
This is great! Thanks for putting this together so quickly, and I think you've structured really well - clear what is being done and it's done in a way that allows edits in the future.
If this has been tested and works as expected, happy for this to go in 👍🏻
@@ -35,10 +35,20 @@ | |||
# File descriptor and clock | |||
_RE_SP3_HEAD_FDESCR = _re.compile(rb"\%c[ ]+(\w{1})[ ]+cc[ ](\w{3})") | |||
|
|||
# Nodata ie NaN constants for SP3 format | |||
SP3_CLOCK_NODATA_STRING = " 999999.999999" # Not used for reading, as fractional components are optional |
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 like the addition of these at the top, we can then just change it here in future
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 was also motivated to do it because I think it makes it clearer what is being done when a value is e.g. used several layers into some calculation logic.
@@ -317,44 +340,51 @@ def clk_log(x): | |||
def prn_formatter(x): | |||
return f"P{x}" | |||
|
|||
# TODO NOTE |
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.
No note left here?
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.
Ah, I added this line just to more clearly draw attention to the large block of commentary already there.
Specifically the fact we're not precicely following the SP3 standard, as we have whitespace between columns. If we have reason to rework our SP3 writing in future, this feels like an important motivator to include in the discussion.
# invoke them on NaN values!! As such, trying to handle NaNs in the formatter is a fool's errand. | ||
# Instead, we do it here, and get the formatters to recognise and skip over the already processed nodata values | ||
|
||
# POS nodata formatting |
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.
Awesome, this whole section reads well - pretty clear what is being done and in a logical manner 👍🏻
No description provided.