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

Support for new linkbcs + handful of smaller changes #544

Merged
merged 51 commits into from
Apr 25, 2024

Conversation

sshakoor1
Copy link
Contributor

@sshakoor1 sshakoor1 commented Nov 28, 2023

The original intent of this PR was to modify gcm_run.j and gcm_setup to have the model point to the new boundary condition directories created by @biljanaorescanin for all runs. Since then it has become bigger than intended and taken on a multitude of features.

Some of the key features of this PR are as follows:

  • To match the new naming conventions used by the land people, Icarus is now referred to as ICA and Icarus-NLv3 is now NL3.
  • linkbcs is now it's own file. It is no longer created inside of gcm_run.j, instead it is now created during gcm_setup by templating a file called linkbcs.tmpl.
  • A new boundary condition, v11, has been added.
  • CatchmentCN is no longer bound to NL3, rather, catchment is ran regardless of which BC is chosen.
  • regrid.pl is no longer supported. It has been replaced with remap_restarts.py inside of gcm_run.j for EMIP runs.
  • Additional clean up of the ugliness that is gcm_run.j and gcm_setup
  • Changes reflected in NCCS, NAS, AWS, and desktop.

NOTE: Coupled MIT ocean runs are currently unsupported.

@sshakoor1 sshakoor1 added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Nov 28, 2023
@sshakoor1 sshakoor1 self-assigned this Nov 28, 2023
@sanAkel
Copy link
Collaborator

sanAkel commented Nov 28, 2023

@sshakoor1 I strongly recommend you put this aside until #538 is merged or coordinate your work with above PR 538.

@mathomp4 , @sdrabenh and I want to lock up three configurations - you'll have to test all of them, while @sdrabenh is working through it all.

@biljanaorescanin
Copy link
Contributor

So far I've tested these cases on NCCS from gcm_setup: NL3 o1 c180, v11 o2 c90, and MOM6 for c12, c90 and c180 all were zero diff to baseline (current develop branch). I was testing all cases with defaults set for other options. And only AMIP runs.

@sshakoor1 If you want me to test those other LSM options, you must add those changes to the PR. I've sent you chat for MIT at C90 error since I am not sure is it my setup.

@mathomp4
Copy link
Member

I've pushed a new set of CI images that I think should let this pass CI. We shall see. To test, I removed what I think was an stray copy-paste in gcm_run.j (wasn't doing anything).

gcm_setup Outdated Show resolved Hide resolved
@biljanaorescanin
Copy link
Contributor

Two more comments:

  1. we also said we will update this line

    $GEOSBIN/regrid.pl -np -ymd ${year}${month}${day} -hr 21 -grout C${AGCM_IM} -levsout ${AGCM_LM} -outdir . -d . -expid $RSTID -tagin @EMIP_BCS_IN -oceanin e -i -nobkg -lbl -nolcv -tagout @LSMBCS -rs 3 -oceanout @OCEANOUT

    To use new package remap_restarts.py. Any update on that part?

  2. we are waiting on @afahadabdullah to update us on MIT option status, if there are any issues. I did get some fails but I was unsure was the issue with my setup.

…M/GEOSgcm_App into feature/sshakoor/linking_new_bcs
@biljanaorescanin
Copy link
Contributor

Short summary testing ( 1 day runs) and current PR status:

  1. output from AMIP at C24 with CatchCN CLM 4.0 NL3 is zero diff to what we get from develop
  2. outputs from AMIP, replay and inc. replay with Catch NL3 were all zero diff to develop
  3. output from coupled MOM6 C12 run is zero diff to develop
  4. output from AMIP Catch ICA run is zero diff to develop
  5. I can confirm, option CatchCN CLM 4.5 was removed from gcm_setup
  6. MIT grid is under testing by @afahadabdullah
  7. Plan is to test EMIP run with upcoming v11 bcs land testing done by @sdrabenh

I didn't try CatchCN CLM 4.0` with ICA @gmao-jkolassa should I test that as well?

Once we confirm EMIP runs as expected and no other changes will be made @sshakoor1 will change other setup scripts as well. I hope I didn't miss anything important to mention.

@gmao-jkolassa
Copy link
Contributor

@biljanaorescanin Thanks for running the Catchment-CN tests! As far as I am aware all current applications of Catchment-CN use NL3 or newer. So from my end, there is no need to test ICA.

@sshakoor1
Copy link
Contributor Author

sshakoor1 commented Feb 22, 2024

@biljanaorescanin looks good to me. Once 6. and 7. are complete and the other setup scripts are changed we should be ready to finally merge this into main!

@yvikhlya
Copy link

Tested this with GEOSgcm_GridComp from develop and MOM5, it works.

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin Thanks for running the Catchment-CN tests! As far as I am aware all current applications of Catchment-CN use NL3 or newer. So from my end, there is no need to test ICA.

@biljanaorescanin @gmao-jkolassa : Are we allowing the use of ICA bcs with CatchCNCLM40? (Sorry, I keep forgetting which bcs versions should work with CatchCNCLM40.) If yes, we should run the test to make sure that setup puts everything in place correctly for this model/bcs combination. I would expect a 0-diff comparison if setup handles the bcs correctly, but the latter should probably still be verified. Having said that, if we think nobody does run or ever again will run CatchCNCLM40 with ICA bcs, then maybe it would be better to disallow the use of ICA bcs with CatchCNCLM40. Either option is fine with me.

@gmao-jkolassa
Copy link
Contributor

@gmao-rreichle All newer Catchment-CN4.0 experiments are using NL3 and I don't think anyone setting up a new experiment with Catchment-CN4.0 would have a reason to go back to an older bcs version. Also, ICA was never used in Catchment-CN4.0 experiments, so it is not needed to reproduce previous results. So, it is not necessary to keep the ICA option, but there is also nor harm in keeping it. It might make sense to make this decision depending on whether it is easier to (1) test ICA for Catchment-CN4.0 or (2) disable the ICA option for Catchment-CN4.0. I don't have a strong preference either way.

@biljanaorescanin
Copy link
Contributor

@sdrabenh were there any changes you had to do on top of this branch to make EMIP run?

@biljanaorescanin
Copy link
Contributor

@sshakoor1 we need draft label removed from PR. And you have one file that needs to be removed ".AGCM.rc.tmpl.swp" .
@sdrabenh than it is in your hands, I think we abandoned for now MIT grid but @sshakoor1 can confirm status of that.

@sshakoor1 sshakoor1 marked this pull request as ready for review April 24, 2024 16:07
@sshakoor1 sshakoor1 requested a review from a team as a code owner April 24, 2024 16:07
@sdrabenh
Copy link
Collaborator

@sshakoor1 thanks for removing the swap file. Also, can you please apply the same changes you made to gcm_run.j to gcm_forecast.tmpl? Thanks

@sshakoor1
Copy link
Contributor Author

@sdrabenh sure, it may take me a day or so to transfer the changes so bare with me.

@sshakoor1
Copy link
Contributor Author

@sdrabenh I pushed an update gcm_forecast.tmpl. I'm not sure how to run forecast runs, so could you please test it before we merge?

@mathomp4
Copy link
Member

@sdrabenh I'll take a shot at merging over the gcm_setup changes to all the other _setup scripts. The scripts are both just different and just similar enough that merging over the code is ... confusing.

Note to @mmanyin, when I'm done, I'll ping you and maybe you can take a look and make sure I don't break the stratchem and gmichem scripts.

@sdrabenh
Copy link
Collaborator

@sdrabenh I'll take a shot at merging over the gcm_setup changes to all the other _setup scripts. The scripts are both just different and just similar enough that merging over the code is ... confusing.

Note to @mmanyin, when I'm done, I'll ping you and maybe you can take a look and make sure I don't break the stratchem and gmichem scripts.

Thanks @mathomp4 @mmanyin. Just let me know when you think it correct and I'll pull in

@sdrabenh
Copy link
Collaborator

@sdrabenh I pushed an update gcm_forecast.tmpl. I'm not sure how to run forecast runs, so could you please test it before we merge?

Looks good to me except I added the line to copy linkbcs to the directory

@sdrabenh sdrabenh merged commit b575966 into develop Apr 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants