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

Phase 2 validation: updates for msonet yamls #188

Merged
merged 29 commits into from
Nov 21, 2024

Conversation

delippi
Copy link
Collaborator

@delippi delippi commented Oct 10, 2024

Description

Finished phase 2 testing with great results. The main additions were updates gross error check and better use of QualityMarker. For winds, I added all the providers and specific stations from each provider similarly to the GSI mesonetuselist and mesonet_stnuselist.

Issue(s) addressed

Results are documented in Issue #80

Dependencies

List the other PRs that this PR is dependent on:

Sample Result

Here is the combined test for all msonet data.
fv3jedi_vs_gsi_increment_airTemperature_msonet_all

…updates gross error check and better use of QualityMarker. For winds, I added all the providers and specific stations from each provider similarly to the GSI mesonetuselist and mesonet_stnuselist.
@delippi delippi self-assigned this Oct 10, 2024
@ShunLiu-NOAA
Copy link

@guoqing-noaa and @delippi Thank you for the discussion. I thought we could use a simpler naming convention. However, as Donnie mentioned we may have to follow JEDI's style at moment. I was told that JEDI is using the naming convention that can be expanded to include model variables beyond atmospheric model.

@guoqing-noaa
Copy link
Collaborator

guoqing-noaa commented Oct 11, 2024

@guoqing-noaa and @delippi Thank you for the discussion. I thought we could use a simpler naming convention. However, as Donnie mentioned we may have to follow JEDI's style at moment. I was told that JEDI is using the naming convention that can be expanded to include model variables beyond atmospheric model.

@ShunLiu-NOAA Thanks for input. That's a different story where JEDI tries to follow the CCPP naming convention for model variables (i.e. keywords). Here obs_space.name is just a mark for us to distinguish different obs_space in the merged yaml file and actually the most important part is the prepbufr type (e.g. 181, 120, 133, etc). If we can tell what the obs space is at a glance, that's far enough.

@delippi delippi marked this pull request as draft October 11, 2024 14:36
@delippi
Copy link
Collaborator Author

delippi commented Oct 11, 2024

Aside from the discussion above, I found some other things that I missed before. Changed PR to draft mode.

@hu5970
Copy link
Contributor

hu5970 commented Oct 11, 2024

H All, I think we better to follow general JEDI naming convention. It is long but it is clear for all of the users.

hu5970
hu5970 previously approved these changes Oct 11, 2024
@delippi
Copy link
Collaborator Author

delippi commented Oct 11, 2024

  1. msonet_wind_288.yaml
  2. msonet_windComponents_288.yaml
  3. msonet_windEastNorth_288.yaml
  4. msonet_windEastNorthward_288.yaml

@hu5970 @ShunLiu-NOAA @guoqing-noaa Thank you all for the input. If we do try to follow the JEDI naming convention, what is the preferred name to replace uv in msonet_uv_288.yaml? The actual names are windEastward and windNorthward, but I thought msonet_windEastward_windNorthward.yaml was too long. Maybe that is the preferred?

@HuiLiu-NOAA
Copy link
Collaborator

HuiLiu-NOAA commented Oct 11, 2024 via email

@HuiLiu-NOAA
Copy link
Collaborator

Is there a reason to assimilate only "windEastNorth_288"?

@HuiLiu-NOAA
Copy link
Collaborator

What I meant is that "msonet_windEastNorth_288" is NOT clear.

@delippi
Copy link
Collaborator Author

delippi commented Oct 11, 2024

Thanks @HuiLiu-NOAA. msonet_winds_288.yaml sounds good to me if that is what is typically used in JEDI to refer to both. I wasn't sure if that was already established.

Is there a reason to assimilate only "windEastNorth_288"?
What I meant is that "msonet_windEastNorth_288" is NOT clear.

I'm not sure that I follow what you mean. I'm not assimilating only wind from 288. I have each obtype from each observing network in its own yaml. So for mesonet, I will have:

msonet_airTemperature_188.yaml
msonet_stationPressure_188.yaml
msonet_specificHumidity_188.yaml
msonet_winds_288.yaml

Is that is what you mean?

I think this is the easiest approach because it keeps each yaml as simple as possible and facilitates easier testing, debugging, and reusing validated yamls for new obtypes (since a lot of each content can be carried over to other types). I also know that when I'm editing msonet_uv_288.yaml I'm only making edits to that obtype. There are no unintended updates to other types. Plus I think it really simplifies the yamls. For validation/testing purposes, it is probably easiest to debug a single obtype from a single platform at a time rather than trying to do it all at once.

@guoqing-noaa
Copy link
Collaborator

If you really want to follow the JEDI naming convention, please check the latest JEDI naming effort and make sure everything is consistent with them.

For example, check this PR:
https://github.com/JCSDA-internal/oops/issues/2737

@HuiLiu-NOAA
Copy link
Collaborator

HuiLiu-NOAA commented Oct 11, 2024

@delippi : the new names look fine to me.

PS: I guess the yamls for each of the obs types are mainly for current initial testing only. For future operational, it may be better to have a consolidated and complete single yaml file with all of the obs types under the same "obs space"as @guoqing mentioned.

@guoqing-noaa
Copy link
Collaborator

I think they will make the following change:

specific_humidity ->
water_vapor_mixing_ratio_wrt_moist_air OR
water_vapor_mixing_ratio_wrt_moist_air_and_condensed_water```

@guoqing-noaa
Copy link
Collaborator

For surface temperature observations, please change as follows:

surface_temperature -> air_temperature_at_2m

@guoqing-noaa
Copy link
Collaborator

for surface humidity observations, please change as follows:

water_vapor_mixing_ratio_wrt_moist_air_at_2m OR
water_vapor_mixing_ratio_wrt_moist_air_and_condensed_water_at_2m

@delippi
Copy link
Collaborator Author

delippi commented Oct 11, 2024

@delippi : the new names look fine to me.

PS: I guess the yamls for each of the obs types are mainly for current initial testing only. For future operational, it may be better to have a consolidated and complete single yaml file with all of the obs types under the same "obs space"as @guoqing mentioned.

@HuiLiu-NOAA, maybe. I'm pretty sure the long term solution is JCB. But even without JCB, I would still argue it is easiest without consolidating them how I currently have them. I still don't understand the reason why we would want to consolidate all these types into a single yaml. It makes them way too complicated and very prone to error.

@delippi
Copy link
Collaborator Author

delippi commented Oct 11, 2024

I think they will make the following change:

specific_humidity ->
water_vapor_mixing_ratio_wrt_moist_air OR
water_vapor_mixing_ratio_wrt_moist_air_and_condensed_water```

These aren't exactly what we are talking about. In the JEDI yamls we use the following specificHumidity, airTemperature, windEastward, windNorthward, stationPressure. The only question is what is used to refer to BOTH windEastward and windNorthward which @HuiLiu-NOAA mentioned it should just be winds.

@ShunLiu-NOAA
Copy link

@delippi Is there anything else that needs to be changed with the PR? Or are you waiting for the reviewers' comments?

@delippi
Copy link
Collaborator Author

delippi commented Nov 13, 2024

@ShunLiu-NOAA, I think we should wait until I finalize some other PRs I have in ufo. There will be some updates needed to these yamls to invoke some of the options that I am implementing. Here are those ufo PRs I'm currently working on.

https://github.com/JCSDA-internal/ufo/pull/3512
https://github.com/JCSDA-internal/ufo/pull/3526

@delippi
Copy link
Collaborator Author

delippi commented Nov 14, 2024

@ShunLiu-NOAA, I think https://github.com/JCSDA-internal/ufo/pull/3512 is the only one we needed to wait for. I had approval for it yesterday but had to change some things with the coding norms. The naming of the new options are finalized now, so I think I'm done with everything here. The only hold up now will be if we want to wait for both of those UFO PRs to be merged and then sync the submodules in RDASApp first? The yamls will "probably" work functionally until then. I don't think it will be too much longer on those PRs.

@ShunLiu-NOAA
Copy link

@delippi Thanks for the update. Let's wait for UFO PRs to be merged and then sync submodules in RDASApp.

@delippi
Copy link
Collaborator Author

delippi commented Nov 15, 2024

@ShunLiu-NOAA, looks like JCSDA fv3-jedi and ufo develop branches have all of my updates. I'll work on updating the submodules with these changes.

@delippi
Copy link
Collaborator Author

delippi commented Nov 19, 2024

This comment #80 (comment) shows the most up-to-date results. Overall, I think everything looks good to go. The ob counts are a bit lower for stationPressure. I can change it from PT06M to PT00M to get more obs, but that seems counter productive to removing duplicate obs. Furthermore, it doesn't change results much just overall ob counts.

Copy link
Contributor

@SamuelDegelia-NOAA SamuelDegelia-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for all of the effort @delippi. We'll probably continue updating the obs-space sections of the yamls a ton during development. So I think its best to merge this now and do separate PRs for any additional changes

@ShunLiu-NOAA ShunLiu-NOAA merged commit bbfb811 into NOAA-EMC:develop Nov 21, 2024
1 check passed
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.

7 participants