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

Added command line feature for remap_restarts.py #19

Merged
merged 96 commits into from
Nov 17, 2023

Conversation

weiyuan-jiang
Copy link
Contributor

This PR added command line features. The interface also changed. To get help, please use
./remap_restarts.py -h
./remap_restarts.py config_file -h
./remap_restarts.py command_line -h

@weiyuan-jiang weiyuan-jiang added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label May 23, 2023
@weiyuan-jiang
Copy link
Contributor Author

@biljanaorescanin Thanks for helping testing. Please save some examples so I can added to the test cases

@mathomp4
Copy link
Member

mathomp4 commented May 23, 2023

@weiyuan-jiang You should change to use the choices argument for argparser. For example, in my create_expt.py code I have:

        p.add_argument('--moist', help="Moist microphysics to use (Default: BACM)", type=str, default='BACM',
                choices=['BACM','GFDL','MGB2'])

and when I run create_expt.py -h I see:

  --moist {BACM,GFDL,MGB2}
                        Moist microphysics to use (Default: GFDL)

So argparse sort of automatically fills out the allowed choices which is nice and restricts it so you can't pass in unallowed options:

❯ create_expt.py test-c12 --moist ABCD
usage: create_expt.py [-h] [-v] [-q] [--expdsc EXPDSC] [--expdir EXPDIR]
...
create_expt.py: error: argument --moist: invalid choice: 'ABCD' (choose from 'BACM', 'GFDL', 'MGB2')

You can equivalently do something like:

        moist_choices = ['BACM','GFDL','MGB2']
        p.add_argument('--moist', help="Moist microphysics to use (Default: BACM)", type=str, default='BACM',
                choices=moist_choices)

I'm never quite sure when's the right time to make a separate array. I suppose if you can use the array in more than one place...

@weiyuan-jiang
Copy link
Contributor Author

Great.

@gmao-rreichle gmao-rreichle self-requested a review May 24, 2023 14:06
@weiyuan-jiang
Copy link
Contributor Author

weiyuan-jiang commented May 26, 2023

The last commit addressed the issue #20 . Ben's scripts are slightly modified and plugged into this package. @bena-nasa I didn't take catchment's conversion because it is already converted during regridding.

gmao-rreichle
gmao-rreichle previously approved these changes Nov 14, 2023
@gmao-rreichle
Copy link
Contributor

@weiyuan-jiang can you add a notebook (clear output before pushing to github) to show at least one use case, for e.g., remap_upper.py? Thanks!

@sanAkel -- we have updated the Wiki page: https://github.com/GEOS-ESM/GEOS_Util/wiki/remap_restarts-package and ironed out a few more kinks (incl. making the package ready for NAS). Please take another look and see if this works for you. If so, please approve, so we can put this on @sdrabenh's desk. I'll defer to @sdrabenh on whether a notebook is needed, and we can always add it later. My preference would be to move this forward before a potential shutdown. Note also that we provide pre-configured test cases / examples in the ./test dir. Thanks!

cc: @weiyuan-jiang @biljanaorescanin

gmao-rreichle
gmao-rreichle previously approved these changes Nov 14, 2023
Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

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

re-approving after bug fix

@biljanaorescanin
Copy link
Contributor

@weiyuan-jiang we will need few more changes to run this on NAS. We did rsync BCS files but seems they use PBS to submit jobs there and now our script is SBATCH hardcoded.

For everything else this PR was tested:

  1. automatic tests that are part of package ( this will work only on discover since NAS doesn't have baseline tests for comparison. )
  2. 12 more random cases which also included few coupled cases where we just change resolution.

I couldn't find any issues. @sdrabenh let me know if you find something during your testing.

@sdrabenh sdrabenh dismissed sanAkel’s stale review November 17, 2023 14:26

Wiki looks reasonable for PR approval

@sdrabenh
Copy link
Contributor

@mathomp4 can you approve on behalf of cmake-team? @bena-nasa doesn't appear to have permissions

@mathomp4
Copy link
Member

@mathomp4 can you approve on behalf of cmake-team? @bena-nasa doesn't appear to have permissions

Odd. He definitely should! I'll look into that next week

@sanAkel
Copy link
Contributor

sanAkel commented Nov 17, 2023

@weiyuan-jiang can you add a notebook (clear output before pushing to github) to show at least one use case, for e.g., remap_upper.py? Thanks!

@sanAkel -- we have updated the Wiki page: https://github.com/GEOS-ESM/GEOS_Util/wiki/remap_restarts-package and ironed out a few more kinks (incl. making the package ready for NAS). Please take another look and see if this works for you. If so, please approve, so we can put this on @sdrabenh's desk. I'll defer to @sdrabenh on whether a notebook is needed, and we can always add it later. My preference would be to move this forward before a potential shutdown. Note also that we provide pre-configured test cases / examples in the ./test dir. Thanks!

cc: @weiyuan-jiang @biljanaorescanin

Thanks @gmao-rreichle wiki looks self-sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mea culpa: MERRA2 restarts are 03z, 09z, 15z and 21z
7 participants