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

Add the "state of ground" to bring in the extra snow depth reports #1368

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions parm/snow/ioda_bufr_python_encoder.py
Copy link
Contributor

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import numpy as np
import bufr
from pyioda.ioda.Engines.Bufr import Encoder


def mask_container(container, mask):
new_container = bufr.DataContainer()
for var_name in container.list():
print(f" ... variable name: {var_name} ...")
var = container.get(var_name)
paths = container.get_paths(var_name)
new_container.add(var_name, var[mask], paths)

return new_container

def create_obs_group(input_path):
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

YAML_PATH = "./obs/bufr_sfcsno_mapping.yaml"
container = bufr.Parser(input_path, YAML_PATH).parse()

sogr = container.get('variables/groundState')
snod = container.get('variables/totalSnowDepth')
snod[(sogr <= 11.0) | (sogr == 15.0)] = 0.0
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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.

container.replace('variables/totalSnowDepth', snod)

print(f" ... Remove filled/missing snow values ...")
masked_container = mask_container(container, (~snod.mask))

encoder = Encoder(YAML_PATH)
data = next(iter(encoder.encode(masked_container).values()))
Copy link
Collaborator

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 :)

Copy link
Collaborator Author

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.


return data
17 changes: 10 additions & 7 deletions parm/snow/obs/config/bufr_sfcsno_mapping.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ bufr:
query: "[*/CLON, */CLONH]"
stationIdentification:
query: "*/RPID"

stationElevation:
query: "[*/SELV, */HSMSL]"

Expand All @@ -26,11 +25,8 @@ bufr:
query: "[*/SNWSQ1/TOSD, */MTRMSC/TOSD, */STGDSNDM/TOSD]"
transforms:
- scale: 1000.0
filters:
- bounding:
variable: totalSnowDepth
lowerBound: 0
upperBound: 10000000
Copy link
Collaborator

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.?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

groundState:
query: "[*/GRDSQ1/SOGR, */STGDSNDM/SOGR]"

encoder:
variables:
Expand Down Expand Up @@ -65,11 +61,18 @@ encoder:
coordinates: "longitude latitude"
source: variables/stationIdentification
longName: "Identification of Observing Location"
units: "m"
units: "index"

# ObsValue
- name: "ObsValue/totalSnowDepth"
coordinates: "longitude latitude"
source: variables/totalSnowDepth
longName: "Total Snow Depth"
units: "mm"

- name: "ObsValue/groundState"
coordinates: "longitude latitude"
source: variables/groundState
longName: "STATE OF THE GROUND"
units: "index"

6 changes: 4 additions & 2 deletions parm/snow/obs/config/sfcsno_snow.yaml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
halo size: 250e3
obsdatain:
engine:
type: bufr
type: script
script file: "{{ PARMgfs }}/gdas/snow/ioda_bufr_python_encoder.py"
args:
input_path: '{{ DATA }}/obs/{{ OPREFIX }}sfcsno.tm00.bufr_d'
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

obsfile: '{{ DATA }}/obs/{{ OPREFIX }}sfcsno.tm00.bufr_d'
Copy link
Collaborator

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...).

Copy link
Contributor

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.

mapping file: '{{ DATA }}/obs/bufr_sfcsno_mapping.yaml'
obsdataout:
engine:
type: H5File
Expand Down
2 changes: 1 addition & 1 deletion sorc/bufr-query
Submodule bufr-query updated 36 files
+4 −2 CMakeLists.txt
+5 −5 cmake/Targets.cmake
+4 −2 core/CMakeLists.txt
+8 −0 core/include/bufr/BufrParser.h
+25 −2 core/include/bufr/DataContainer.h
+523 −1 core/include/bufr/DataObject.h
+5 −7 core/include/bufr/DataProvider.h
+8 −3 core/include/bufr/File.h
+3 −0 core/include/bufr/NcepDataProvider.h
+3 −0 core/include/bufr/WmoDataProvider.h
+66 −1 core/src/bufr/BufrReader/BufrParser.cpp
+51 −11 core/src/bufr/BufrReader/Query/DataProvider/DataProvider.cpp
+8 −0 core/src/bufr/BufrReader/Query/DataProvider/NcepDataProvider.cpp
+7 −0 core/src/bufr/BufrReader/Query/DataProvider/WmoDataProvider.cpp
+12 −5 core/src/bufr/BufrReader/Query/File.cpp
+56 −0 core/src/bufr/DataContainer.cpp
+11 −2 core/src/encoders/netcdf/Encoder.cpp
+31 −0 docs/python_api.rst
+1 −0 docs/uml/BUFR_BigPicture.puml
+25 −28 python/CMakeLists.txt
+17 −10 python/DataObjectFunctions.cpp
+4 −4 python/DataObjectFunctions.h
+4 −0 python/py_bufr.cpp
+17 −1 python/py_data_container.cpp
+2 −1 python/py_file.cpp
+49 −0 python/py_mpi.cpp
+23 −0 python/py_mpi.h
+15 −2 python/py_parser.cpp
+12 −8 test/CMakeLists.txt
+2 −2 test/testinput/bufrtest_fieldname_validation.py
+88 −0 test/testinput/bufrtest_python_mpi_test.py
+21 −12 test/testinput/bufrtest_python_test.py
+3 −0 tools/bufr2netcdf/CMakeLists.txt
+140 −44 tools/bufr2netcdf/bufr2netcdf.cpp
+1 −1 tools/build_scripts/bufr_query_cpp_lint.py
+11 −3 tools/show_queries/QueryPrinter.cpp
Loading