-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add the "state of ground" to bring in the extra snow depth reports #1368
base: develop
Are you sure you want to change the base?
Conversation
@@ -2,3 +2,4 @@ mkdir: | |||
- '{{ DATA }}/obs' | |||
copy: | |||
- ['{{PARMgfs}}/gdas/snow/obs/config/bufr_sfcsno_mapping.yaml', '{{ DATA }}/obs/'] | |||
- ['{{PARMgfs}}/gdas/snow/obs/config/ioda_bufr_python_encoder.py', '{{ DATA }}'] |
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.
does the python script need copied? and should it go here? we might need to talk to @emilyhcliu about the progress of SPOC
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.
Thanks @CoryMartin-NOAA for your suggestions. Do you agree if I put this python code to sorc/gdas.cd/parm/snow/
, so that this code can be used directly?
@emilyhcliu Any suggestions?
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 moved this py code to the parm/gdas/snow/ directory for a temporary solution. @emilyhcliu Please let me know if you have any other suggestions. Thanks.
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.
Scripts shouldn't be in parm/, until we move to SPOC, it should probably go here: https://github.com/NOAA-EMC/GDASApp/tree/develop/ush/ioda/bufr2ioda
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.
Understood. Wait for @emilyhcliu suggestions.
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.
Thanks for doing this @jiaruidong2017 . I have requested one small change.
|
||
sogr = container.get('variables/groundState') | ||
snod = container.get('variables/totalSnowDepth') | ||
snod[(sogr <= 11.0) | (sogr == 15.0)] = 0.0 |
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.
Without having seen the data, I think there should be an if statement here - so that we only replace the snod when it was undefined / missing.
i.e., if snod > 0. (or some, slighty higher threshold), and sogr indictaes no snow cover, I assume we would want to use the snod value, not 0.
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.
Above (with an if statement) is all consistent with how the PR description.
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.
Good suggestions. I will investigate this.
@rmclaren Do you have any suggestions?
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.
Thanks again for the good findings. I modified to only replace the filled/missing snod.
- bounding: | ||
variable: totalSnowDepth | ||
lowerBound: 0 | ||
upperBound: 10000000 |
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.
For my understanding:
- is this introducing a 10 m limit on snow depth obs?
- if they are above 10 m, are they discarded, or set to 10.?
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 previously put this large number (10000m) here for removing the missing values. Here, we don't need this any more because we need to set the missing snod values to 0 when sogr satisfies the defined conditions.
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 might still have missing values though wouldn't we?
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.
The missing values will be removed after applying sogr conditions.
|
||
return new_container | ||
|
||
def create_obs_group(input_path): |
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.
Some comments here would be helpful to explain what we're doing - specifically what snogr is, and why we're selecting out the values that we are.
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.
Excellent suggestions. I will add the comments here later. Thanks.
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.
Add the comments to explain the steps.
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.
@jiaruidong2017 move this to https://github.com/NOAA-EMC/GDASApp/tree/develop/ush/ioda/bufr2ioda but change its name to something more descriptive/specific until we can move things to SPOC
encoder = Encoder(YAML_PATH) | ||
data = next(iter(encoder.encode(masked_container).values())) |
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.
Could be 1 liner data = next(iter(Encoder(YAML_PATH).encode(masked_container).values()))
. Up to you :)
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.
Thanks @rmclaren for your suggestions. Will change it.
input_path: '{{ DATA }}/obs/{{ OPREFIX }}sfcsno.tm00.bufr_d' | ||
obsfile: '{{ DATA }}/obs/{{ OPREFIX }}sfcsno.tm00.bufr_d' |
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.
Don't forget to add an issue so they do not assume the formatting of the YAML file (obsfile variable shoould not be necessary). Reading the backend engines YAML breaks encapsulation (external modules should not be dependent on the private configuration for a backend type. Some backends may not have any kind of file associated with them (like a db...).
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.
yes thanks @rmclaren I'm aware of that issue, I/we/others need to figure out how best to handle it on the workflow side.
type: script | ||
script file: "{{ USHgfs }}/bufr2ioda_sfcsno_bufr_encoder.py" | ||
args: | ||
input_path: '{{ DATA }}/obs/{{ OPREFIX }}sfcsno.tm00.bufr_d' |
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.
Suggestion:
Make the mapping file as input to the Python script for create_obs_group
args:
input_path: '{{ DATA }}/obs/{{ OPREFIX }}sfcsno.tm00.bufr_d'
mapping_path: = "./obs/bufr_sfcsno_mapping.yaml"
This will make it clear that the script backend requires two inputs: bufr file and the mapping file
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.
Thanks @emilyhcliu for the suggestions. I made the changes as suggested.
@CoryMartin-NOAA For v17 global-workflow, do we want to read the bufr as the backend (this PR), or do we want to convert the bufr info IODA format netCDF in the obsproc step? |
@emilyhcliu I think my preference would be backend for this (and other) case where the input file is BUFR. |
@jiaruidong2017 Have you run a cycle in global-workflow with the changes from this PR? |
@jiaruidong2017 Could you point me where to get a sfcsno bufr data for testing? Thanks. |
@emilyhcliu You can find the data on hera from the dump directory at: |
Yes, it worked as expected. |
Oh, no wonder I could not find the near-real-time data. Thanks!! |
@jiaruidong2017 I tested the Python script for converter for 2021080100 sfcsno bufr
The final IODA output file has data account = 3685 This is the result of my test. Do those data count values look right to you? |
The real-time data will be available soon (expected in next January) after the next release of |
Yeah, it looks good to me. Thanks. |
@emilyhcliu You can find the near-real-time
|
This PR uses the python BUFR2IODA API to read the GTS bufr snow observations and adds to read the
state of ground
(SOGR) for bringing in the extra snow depth reports. If the state of the ground says there is no snow, but the snow depth observation is missing, we should change the missing values to be 0 so that the reports can be assimilated.This PR also incorporates the latest changes from submodule from
NOAA-EMC/bufr-query
.This PR contributes to NOAA-EMC/global-workflow#3093