search

NCAR / MPAS-Workflow

Scripts for controlling DA workflows with MPAS-Model and mpas-bundle
Apache License 2.0
20 stars 15 forks source link

mods to get iasi and cris obs #300

Closed zhumingying closed 7 months ago

zhumingying commented 7 months ago

Description

Modify bin/ObsToIODA.csh to get the obs for IASI and CrIS. Getting obs of IASI and CrIS need more memory and CPU time, initialize/data/Observations.py is also changed.

Tests completed

zhumingying commented 7 months ago

I tested for getting conv data only, or iasi or cris only, and all conv, iasi, cris together. We can talk tomorrow after meeting

Thanks! Zhuming

On Tue, Apr 2, 2024 at 12:33 PM ibanos90 @.***> wrote:

@.**** requested changes on this pull request.

Hi @zhumingying https://github.com/zhumingying, sorry for the delay in reviewing this PR! I took a look at the log of this branch and it needs to be updated (previous commit before your changes is c1e6754 https://github.com/NCAR/MPAS-Workflow/commit/c1e6754cb13e4a45d410a5ff27df1947d9eefc2e), can you merge it with develop? Also, could you provide more information in the description and title of the PR? Did the test pass with these changes? I noted that some of your changes in bin/ObsToIODA.csh may not be necessary related to cris or iasi, is there a reason for that? Seems like you changed the logic for the upgrade part.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/MPAS-Workflow/pull/300#pullrequestreview-1974626321, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFASISOL56LFW4QSQIUZULDY3L2YLAVCNFSM6AAAAABFLE75UOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZUGYZDMMZSGE . You are receiving this because you were mentioned.Message ID: @.***>

junmeiban commented 7 months ago

@zhumingying Thank you so much for adding the new obs and fix the logic. Once you remove the line 143 gnssro, I will approve it.

zhumingying commented 7 months ago

done

On Wed, Apr 3, 2024 at 4:15 PM Junmei Ban @.***> wrote:

@zhumingying https://github.com/zhumingying Thank you so much for adding the new obs and fix the logic. Once you remove the line 143 gnssro, I will approve it.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/MPAS-Workflow/pull/300#issuecomment-2035695158, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFASISPR63TVRTQLKSVHLLDY3R5PPAVCNFSM6AAAAABFLE75UOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGY4TKMJVHA . You are receiving this because you were mentioned.Message ID: @.***>

junmeiban commented 7 months ago

done … On Wed, Apr 3, 2024 at 4:15 PM Junmei Ban @.> wrote: @zhumingying https://github.com/zhumingying Thank you so much for adding the new obs and fix the logic. Once you remove the line 143 gnssro, I will approve it. — Reply to this email directly, view it on GitHub <#300 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFASISPR63TVRTQLKSVHLLDY3R5PPAVCNFSM6AAAAABFLE75UOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGY4TKMJVHA . You are receiving this because you were mentioned.Message ID: @.>

Many thanks. I am running scenarios/3dvar_OIE120km_ColdStart.yaml to double check it

junmeiban commented 7 months ago

Hi @zhumingying , Just as @ibanos90 mentioned, you should merge https://github.com/NCAR/MPAS-Workflow/commit/c1e6754cb13e4a45d410a5ff27df1947d9eefc2e

junmeiban commented 7 months ago

Hi @zhumingying , Just as @ibanos90 mentioned, you should merge c1e6754

Sorry, please forget my comments, you already included the commit. But it looks your feature branch is based on release/2.1.0??? So you missed some commits.

zhumingying commented 7 months ago

Hi @ibanos90,

I have made a minor revision to bin/ObsToIODA.csh. The original script processes observations with v1tov2, v2tov3, and ScanPositionUpdate under the condition:

if ( "${convertToIODAObservations}" =~ "prepbufr" || " ${convertToIODAObservations}" =~ "satwnd" ) then

While it's acceptable to process conventional and other observations together, it will halt if conventional observations are not processed but other observations like iasi are.

Thanks! Zhuming

On Tue, Apr 2, 2024 at 12:33 PM ibanos90 @.***> wrote:

@.**** requested changes on this pull request.

Hi @zhumingying https://github.com/zhumingying, sorry for the delay in reviewing this PR! I took a look at the log of this branch and it needs to be updated (previous commit before your changes is c1e6754 https://github.com/NCAR/MPAS-Workflow/commit/c1e6754cb13e4a45d410a5ff27df1947d9eefc2e), can you merge it with develop? Also, could you provide more information in the description and title of the PR? Did the test pass with these changes? I noted that some of your changes in bin/ObsToIODA.csh may not be necessary related to cris or iasi, is there a reason for that? Seems like you changed the logic for the upgrade part.

— Reply to this email directly, view it on GitHub https://github.com/NCAR/MPAS-Workflow/pull/300#pullrequestreview-1974626321, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFASISOL56LFW4QSQIUZULDY3L2YLAVCNFSM6AAAAABFLE75UOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZUGYZDMMZSGE . You are receiving this because you were mentioned.Message ID: @.***>

ibanos90 commented 7 months ago

Hi @ibanos90, I have made a minor revision to bin/ObsToIODA.csh. The original script processes observations with v1tov2, v2tov3, and ScanPositionUpdate under the condition: if ( "${convertToIODAObservations}" =~ "prepbufr" || " ${convertToIODAObservations}" =~ "satwnd" ) then While it's acceptable to process conventional and other observations together, it will halt if conventional observations are not processed but other observations like iasi are. Thanks! Zhuming … On Tue, Apr 2, 2024 at 12:33 PM ibanos90 @.> wrote: @*.*** requested changes on this pull request. Hi @zhumingying https://github.com/zhumingying, sorry for the delay in reviewing this PR! I took a look at the log of this branch and it needs to be updated (previous commit before your changes is c1e6754 <c1e6754>), can you merge it with develop? Also, could you provide more information in the description and title of the PR? Did the test pass with these changes? I noted that some of your changes in bin/ObsToIODA.csh may not be necessary related to cris or iasi, is there a reason for that? Seems like you changed the logic for the upgrade part. — Reply to this email directly, view it on GitHub <#300 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFASISOL56LFW4QSQIUZULDY3L2YLAVCNFSM6AAAAABFLE75UOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZUGYZDMMZSGE . You are receiving this because you were mentioned.Message ID: @.>

Hi @zhumingying, thank you for addressing my comments! I added a last comment on the code. Also, can you remove blank spaces added in bin/ObsToIODA.csh? They show up as red when you do git diff develop. Thanks!

zhumingying commented 7 months ago

"mv -f file1 file " should be equal to " rm -f file; mv file1 file" google search: "mv" is the standard move command that prompts for confirmation when overwriting files, while "mv -f" is used to force the move without asking for confirmation, potentially overwriting existing files.

I just would like to directly accomplish the overwrite without any confirmation prompts.

Zhuming

On Thu, Apr 4, 2024 at 12:19 PM ibanos90 @.***> wrote:

@.**** commented on this pull request.

In bin/ObsToIODA.csh https://github.com/NCAR/MPAS-Workflow/pull/300#discussion_r1552210235:

@@ -109,54 +111,90 @@ foreach gdasfile ( "gdas." ) ln -sfv ${obs2iodaBuildDir}/${obs2iodaEXE} ./ ./${obs2iodaEXE} ${noGSIQCFilters} ../${gdasfile} >&! logs/log-converter_sfc

replace surface obs file with file created without additional QC

  • mv sfcobs${thisCycleDate}.h5 ../sfcobs${thisCycleDate}.h5
  • mv -f sfcobs${thisCycleDate}.h5 ../sfcobs${thisCycleDate}.h5

Is there a reason for adding -f here?

— Reply to this email directly, view it on GitHub https://github.com/NCAR/MPAS-Workflow/pull/300#pullrequestreview-1980803942, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFASISMPL4PFET37RF7262TY3WKRJAVCNFSM6AAAAABFLE75UOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBQHAYDGOJUGI . You are receiving this because you were mentioned.Message ID: @.***>

zhumingying commented 7 months ago

Thanks all!

On Thu, Apr 4, 2024 at 2:56 PM ibanos90 @.***> wrote:

@.**** approved this pull request.

Thanks @zhumingying https://github.com/zhumingying!

— Reply to this email directly, view it on GitHub https://github.com/NCAR/MPAS-Workflow/pull/300#pullrequestreview-1981147180, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFASISOO6MOHQUGGG5HM3NLY3W463AVCNFSM6AAAAABFLE75UOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBRGE2DOMJYGA . You are receiving this because you were mentioned.Message ID: @.***>