-
Notifications
You must be signed in to change notification settings - Fork 15
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
Trublue #271
Trublue #271
Conversation
stglib/core/utils.py
Outdated
@@ -1405,7 +1405,11 @@ def create_water_level_var(ds): | |||
Create water level variable from NAVD88 sensor height | |||
""" | |||
|
|||
if ds.z.attrs["geopotential_datum_name"] == "NAVD88": | |||
if "P_1ac" not in list(ds.data_vars): | |||
print("Cannot create water_level variable without P_1ac") |
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.
If this is an error state you should raise the appropriate error (maybe ValueError?)
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 don't want to throw an error here. I still want the code to finish and produce the netcdf file. This is just a message that will pop up if you are processing without doing the atmospheric correction to let you know there won't be a water_level variable without P_1ac.
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.
Okay. In that case, why is there an exit() call?
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 changed this to: if "P_1ac" in list(ds.data_vars) and ds.z.attrs["geopotential_datum_name"] == "NAVD88":
stglib/core/utils.py
Outdated
print("Cannot create water_level variable without P_1ac") | ||
return ds | ||
exit() | ||
elif ds.z.attrs["geopotential_datum_name"] == "NAVD88": |
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 don't think this should be an elif, right?
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 think it has to be elif because as we talked about last time, z could exist but not be relative to NAVD88, so only if it is relative to NAVD88, the water_level will be created.
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.
We can discuss this offline, but the first part of the conditional has to do with P_1ac, so I'm still unclear on this.
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.
See edit above. P_1ac and z in NAVD88 are required to create water_level. There won't be a P_1ac variable if someone ran the code without the atmospheric pressure correction. But the code still needs to be able to run without the atmospheric pressure correction and not break.
stglib/core/utils.py
Outdated
@@ -1405,7 +1405,11 @@ def create_water_level_var(ds): | |||
Create water level variable from NAVD88 sensor height | |||
""" | |||
|
|||
if ds.z.attrs["geopotential_datum_name"] == "NAVD88": | |||
if "P_1ac" not in list(ds.data_vars): | |||
print("Cannot create water_level variable without P_1ac") |
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.
Okay. In that case, why is there an exit() call?
stglib/core/utils.py
Outdated
print("Cannot create water_level variable without P_1ac") | ||
return ds | ||
exit() | ||
elif ds.z.attrs["geopotential_datum_name"] == "NAVD88": |
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.
We can discuss this offline, but the first part of the conditional has to do with P_1ac, so I'm still unclear on this.
No description provided.