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

Enable MADIS DA #28

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

yuanxue2870
Copy link
Contributor

@yuanxue2870 yuanxue2870 commented Dec 5, 2024

Describe your changes

Summarise all code changes included in PR:

  1. add MADIS yaml
  2. add the MADIS observation directory

List any associated PRs in the submodules.
N/A

Issue ticket number and link

List the git Issue that this PR addresses:
#29

Test output

Is this PR expected to pass the DA_IMS_test (ie., does it change the output)?
Yes.
Does it pass the DA_IMS_test?
Yes.
If changes to the test results are expected, what are these changes? Provide a link to the output directory when running the test:
N/A

Checklist before requesting a review

  • My branch being merged is up to date with the latest develop.
  • I have performed a self-review of my code by examining the differences that will be merged.
  • I have not made any unnecessary code changes / changed any default behavior.
  • My code passes the DA_IMS_test, or differences can be explained.

Copy link
Collaborator

@jiaruidong2017 jiaruidong2017 left a comment

Choose a reason for hiding this comment

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

Suggest to make the changes in consistent with the global-workflow for now.

Also make the corresponding changes to the GTS.yaml. It is better to rename the GTS to the specific name (e.g., sfcsno) indicating the different GTS data sources.

jedi/fv3-jedi/yaml_files/2DVar/MADIS.yaml Outdated Show resolved Hide resolved
where:
- variable:
name: MetaData/stationElevation
minvalue: -999.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
minvalue: -999.0
value: is_valid

Copy link
Contributor Author

@yuanxue2870 yuanxue2870 Dec 13, 2024

Choose a reason for hiding this comment

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

Thanks for pointing this out. I think I would still need to confirm with Clara on this point. For GTS yaml, we used [-200, 9900] as the min and max value. Based on your analysis presented on Thursday 12/12/2024, you would prefer using "is_valid". But the preliminary conclusion from our Thursday meeting seems to be we might need three elevation filters in this case: 1) filtered missing values; 2) filtered unreasonable elevation values outside of the [-200, 9900] range; and 3) filtered elevation difference > 200m. Filters 1) and 2) can be combined, because the missing value is set to a huge number. I have not looked into the MADIS observations in detail to get the range yet. I will do so now. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The min elevation of MADIS stations is typically greater than 0, the max elevation of MADIS stations is often being 4328.2m. However, MADIS does have some unreasonable maximum elevation (other than the missing value set at 9.96e36). I personally think the [-200, 9900] range can also be reused for MADIS. But I will confirm with Clara on Tuesday.

Capture

Copy link
Contributor Author

@yuanxue2870 yuanxue2870 Dec 17, 2024

Choose a reason for hiding this comment

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

Per discussions with Clara on 12/17/2024, will keep doing more tests to make sure that the three filters can all be captured properly in the yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per discussions with the land team on 12/19/2024, no more filter tests are needed. Am adding all three filters.

@yuanxue2870
Copy link
Contributor Author

yuanxue2870 commented Dec 17, 2024

Suggest to make the changes in consistent with the global-workflow for now.

Also make the corresponding changes to the GTS.yaml. It is better to rename the GTS to the specific name (e.g., sfcsno) indicating the different GTS data sources.

Now, "GTS" has been renamed as "SFCSNO" per your suggestion after confirming with Clara on 12/17/2024. Thank you!

@yuanxue2870
Copy link
Contributor Author

All done. Please re-review: @jiaruidong2017 @ClaraDraper-NOAA Thank you!

@ClaraDraper-NOAA
Copy link
Collaborator

Thanks @yuanxue2870 . i'm in the middle of merging a largish PR from Tseganeh - so you may need to deal with some merge conflicts on this one.

@yuanxue2870
Copy link
Contributor Author

Thanks @yuanxue2870 . i'm in the middle of merging a largish PR from Tseganeh - so you may need to deal with some merge conflicts on this one.

OK. I will wait for the /develop updates then.

Copy link
Collaborator

@jiaruidong2017 jiaruidong2017 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks.

@yuanxue2870
Copy link
Contributor Author

Looks good to me. Thanks.

Thanks!

@yuanxue2870
Copy link
Contributor Author

Thanks @yuanxue2870 . i'm in the middle of merging a largish PR from Tseganeh - so you may need to deal with some merge conflicts on this one.

OK. I will wait for the /develop updates then.

Looks like there are no conflicts for this one.

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.

3 participants