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

Rename coupled grids #695

Merged
merged 21 commits into from
Sep 15, 2023
Merged

Rename coupled grids #695

merged 21 commits into from
Sep 15, 2023

Conversation

biljanaorescanin
Copy link
Contributor

@biljanaorescanin biljanaorescanin commented Dec 29, 2022

Rename coupled grids to:
CF0180x6__TP1440x1080; XX = M5, M6 and “TP” stands for Tripolar grid (Ref: https://doi.org/10.1006/jcph.1996.0136)

@sanAkel let me know does this work for you?
I've tested it and it works for us and as you said it is more understandable. And most important no more overwriting with same output name for different oceans. Once we resolve issue with c180 runs we can work on PR to use updated info with gcm_setup.

We are changing in case of T4MOM6 at C180 output from CF0180x6C_TM1440xTM1080 to CF0180x6C_M6TP1440x1080 .
Tile name will go from CF0180x6C_TM1440xTM1080-Pfafstetter.til to CF0180x6C_M6TP1440x1080-Pfafstetter.til

@yvikhlya this is so we don't overwrite outputs of MOM5 and MOM6 with BCS Package for 1440x1080 runs.

@biljanaorescanin biljanaorescanin added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Dec 29, 2022
sanAkel
sanAkel previously approved these changes Dec 29, 2022
Copy link
Contributor

@sanAkel sanAkel 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!

@biljanaorescanin
Copy link
Contributor Author

@sanAkel you don't need to do anything here I just wanted to let you know it will take me a bit of time to coordinate this with other reconstructions of boundary conditions files. I also need to make PR's to address remap_restarts.py and changes in gcm_setup so this can be used

@gmao-rreichle
Copy link
Contributor

We are changing in case of T4MOM6 at C180 output from CF0180x6C_TM1440xTM1080 to CF0180x6C_M6_TP1440x1080' . Tile name will go from CF0180x6C_TM1440xTM1080-Pfafstetter.tiltoCF0180x6C_M6_TP1440x1080-Pfafstetter.til`

And in tile file header will change as well: from PE1440x1080-TM to PE1440x1080-M6_TP

My 2c: Let's avoid the underscore in the "grid" name here. In the bcs output dir tree, we use a single underscore to concatenate the atmos and ocean grid names. If the ocean grid name includes an underscore, the concatenated dir name will be more difficult to understand and parse. My suggestion would be to use strings such as TPMOM5 (or TPM5) and TPMOM6 (or TPM6).
Also, what is the grid on which the MIT ocean GCM runs? Is it also tripolar? If not, we need to be careful not to name the MIT grid "TPMIT".

@sanAkel
Copy link
Contributor

sanAkel commented Apr 5, 2023

My suggestion would be to use strings such as TPMOM5 (or TPM5) and TPMOM6 (or TPM6).

Anyone of those is fine with me.

Also, what is the grid on which the MIT ocean GCM runs?

They are called LLCxx where xx = 90, 4320, ...

Is it also tripolar?

No. Cube sphere. But not same cube as in fv3.

If not, we need to be careful not to name the MIT grid "TPMIT".

Something like LLCMIT?

@gmao-rreichle
Copy link
Contributor

Anyone of those is fine with me.

No. Cube sphere. But not same cube as in fv3.

Something like LLCMIT?

Thanks, @sanAkel! I'm glad I asked. @biljanaorescanin, please keep this info in mind for the PR.

@biljanaorescanin
Copy link
Contributor Author

This last commit will update python scripts to include name change:
going from CF0012x6C_TM0072xTM0036 to CF0012x6C_M6TP-0072x0036

@biljanaorescanin
Copy link
Contributor Author

@gmao-rreichle and @sanAkel, this is ready for you to take a look.

  1. I've removed changes to make_bcs script. Since in meanwhile we moved to only python version BC development.
  2. I tested code after last commit, here is example for C12 T1MOM6 option; geometry directory will be like this:

CF0012x6C_M6TP-0072x0036
├── CF0012x6C_M6TP-0072x0036.j
├── CF0012x6C_M6TP-0072x0036-Pfafstetter.til
├── CF0012x6C_M6TP-0072x0036-Pfafstetter.TRN
├── rst
│ ├── CF0012x6C_M6TP-0072x0036-Pfafstetter.rst
│ ├── CF0012x6C.rst
│ ├── M6TP-0072x0036-Pfafstetter.rst
│ ├── M6TP-0072x0036.rst
│ ├── Pfafstetter-ORIG.rst
│ └── Pfafstetter.rst
└── til
├── CF0012x6C_M6TP-0072x0036-Pfafstetter.til
├── CF0012x6C_M6TP-0072x0036-Pfafstetter.trn
├── CF0012x6C_M6TP-0072x0036-Pfafstetter.TRN
├── CF0012x6C.til
├── M6TP-0072x0036-Pfafstetter.til
├── M6TP-0072x0036.til
├── Pfafstetter-ORIG.til
└── Pfafstetter.til

  1. Whole directory output is restructured to new boundary conditions layout and has new name but files themselves (except the name change diff) are zero diff to what we have on develop for same resolution/grid run.
  2. I've tested few other cases for both MOM5 and MOM6.
  3. We have one silly step we could change to clenup more, that is then we link all MAPL_Tripolar.nc to find one we use as input:

ln -s {MAKE_BCS_INPUT_DIR}/ocean/MOM5/360x200 data/MOM5/360x200
ln -s {MAKE_BCS_INPUT_DIR}/ocean/MOM5/720x410 data/MOM5/720x410
ln -s {MAKE_BCS_INPUT_DIR}/ocean/MOM5/1440x1080 data/MOM5/1440x1080
ln -s {MAKE_BCS_INPUT_DIR}/ocean/MOM6/72x36 data/MOM6/72x36
ln -s {MAKE_BCS_INPUT_DIR}/ocean/MOM6/540x458 data/MOM6/540x458
ln -s {MAKE_BCS_INPUT_DIR}/ocean/MOM6/1440x1080 data/MOM6/1440x1080

Solution could be to have one directory holding all resolutions of MAPL_Tripolar.nc. This nc file is all we use from ocean inputs for the package. We could append resolution/grid name I can search for (for example for C12 T1MOM6) move to something like this MAPL_Tripolar_M6TP_72x36.nc ?

@yvikhlya
Copy link

I would prefer to keep MAPL_Tripolar.nc together with other data files on ocean grid. We could name location for ocean grids like

1440x1080_M6TP

instead of

MOM6/1440x1080

but I don't see much difference.

@biljanaorescanin
Copy link
Contributor Author

This last commit won't change tripolar result. I've just made cleaner loops over ocean options and changed initialization to make it look nicer with UNDEF if option not used.

  • I fixed bug for CF grid name

@gmao-rreichle
Copy link
Contributor

@biljanaorescanin:

This last commit will update python scripts to include name change: going from CF0012x6C_TM0072xTM0036 to CF0012x6C_M6TP-0072x0036

As discussed at our tag-up this morning, please remove the "-" in the ocean grid name convention for consistency. (For the atm grid, we use CF0012x6C and not CF-0012x6C. For the atm stretched-grid qualifier, the "-" is ok.) That is, the string in the example above should read: CF0012x6C_M6TP0072x0036

Also, please edit the first/introductory/summary comment on this PR. The latest edit is from Dec 29, 2022, and the naming conventions in the summary comment are quite outdated.

Other than that, looks good to me.

@biljanaorescanin
Copy link
Contributor Author

Name is changed with last commit. Output geometry structure will be like this:

CF0012x6C_M6TP0072x0036
├── CF0012x6C_M6TP0072x0036.j
├── CF0012x6C_M6TP0072x0036-Pfafstetter.til
├── CF0012x6C_M6TP0072x0036-Pfafstetter.TRN
├── rst
│ ├── CF0012x6C_M6TP0072x0036-Pfafstetter.rst
│ ├── CF0012x6C.rst
│ ├── M6TP0072x0036-Pfafstetter.rst
│ ├── M6TP0072x0036.rst
│ ├── Pfafstetter-ORIG.rst
│ └── Pfafstetter.rst
└── til
├── CF0012x6C_M6TP0072x0036-Pfafstetter.til
├── CF0012x6C_M6TP0072x0036-Pfafstetter.trn
├── CF0012x6C_M6TP0072x0036-Pfafstetter.TRN
├── CF0012x6C.til
├── M6TP0072x0036-Pfafstetter.til
├── M6TP0072x0036.til
├── Pfafstetter-ORIG.til
└── Pfafstetter.til

@biljanaorescanin
Copy link
Contributor Author

@sanAkel did you maybe have a chance to take a look at this? If all is good we should merge this before fix for routing.

@sanAkel
Copy link
Contributor

sanAkel commented Sep 13, 2023

@biljanaorescanin Thanks for alerting me reg this PR.

I was under the impression that this was already merged!

So please go ahead and undraft/ mark it ready for review.

@biljanaorescanin biljanaorescanin marked this pull request as ready for review September 13, 2023 21:28
@biljanaorescanin biljanaorescanin requested review from a team as code owners September 13, 2023 21:28
@sdrabenh sdrabenh merged commit bb053d2 into develop Sep 15, 2023
5 checks passed
@sdrabenh sdrabenh deleted the feature/borescan_grid_name branch September 15, 2023 18:55
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.

5 participants