E3SM-Project / E3SM

Energy Exascale Earth System Model source code. NOTE: use "maint" branches for your work. Head of master is not validated.
https://docs.e3sm.org/E3SM
Other
332 stars 334 forks source link

Adds a new tool to replace gen_domain in CIME #6347

Closed whannah1 closed 2 weeks ago

whannah1 commented 1 month ago

"generate_domain_files_E3SM.py" is a new tool to replace the fortran gen_domain.F90 tool that exists in CIME. The old version was problematic to use because the build configuration was almost always broken when a user went to build it. The new python version works efficiently due to the numba package, which is included in the E3SM unified environment.

[BFB]

github-actions[bot] commented 1 month ago

PR Preview Action v1.4.7 :---: :rocket: Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6347/ on branch gh-pages at 2024-04-24 21:05 UTC

whannah1 commented 4 weeks ago

I think this PR is ready for a thorough review.

I've added documentation for this tool and a new "tools" section of the E3SM docs. I expect other tool documentaiton will show up there soon.

Additionally, I've also started working on recreating the guide for "Running E3SM on New Grids" in the new E3SM docs repo. So there will naturally be some redundant documentation for this tool, but I think that's ok.

singhbalwinder commented 3 weeks ago

Hi @jonbob and @mahf708: Do you guys have any further comments on this PR before I merge it? Thanks!

mahf708 commented 3 weeks ago

I don't think we should have each tool get its own sub-website with mkdocs.yaml. Pinging @rljacob who added the monorepo functionality. Having said that, we can resolve this issue later (it is mostly cosmetic, nothing fundamental), so no need to hold this PR.

jonbob commented 3 weeks ago

@singhbalwinder - I was going to test it and compare output from the Fortran tool quickly, but I'll do that today and post results here

jonbob commented 3 weeks ago

@whannah1 -- it failed for me trying to regenerate a domain file from one of our recent ice/ocn meshes:

python generate_domain_files_E3SM.py -m /lcrc/group/acme/ac.jwolfe/v3-grids/RRS6to18E3r6/map_RRS6to18E3r6_to_TL319_traave.20240403.nc -o RRS6to18E3r6 -l TL319

  Input files and parameter values:
    map_file        : /lcrc/group/acme/ac.jwolfe/v3-grids/RRS6to18E3r6/map_RRS6to18E3r6_to_TL319_traave.20240403.nc
    lnd_grid        : TL319
    ocn_grid        : RRS6to18E3r6
    fminval         : 0
    fmaxval         : 1
    set_omask       : False

  Grid information from map file:
    domain_a file        : ocean.RRS6to18E3r6.scrip.20240402.nc
    domain_b file        : TL319.151007.nc
    ocn_grid_file        : ocean.RRS6to18E3r6.scrip.20240402.nc
    atm_grid_file        : TL319.151007.nc
    ocn grid size   (n_a): 4016094
    atm grid size   (n_b): 204800
    sparse mat size (n_s): 5992418

Traceback (most recent call last):
  File "/lcrc/group/acme/ac.jwolfe/e3sm_PR6347/tools/generate_domain_files/generate_domain_files_E3SM.py", line 360, in <module>
    main()
  File "/lcrc/group/acme/ac.jwolfe/e3sm_PR6347/tools/generate_domain_files/generate_domain_files_E3SM.py", line 211, in main
    add_metadata(ds_out)
  File "/lcrc/group/acme/ac.jwolfe/e3sm_PR6347/tools/generate_domain_files/generate_domain_files_E3SM.py", line 301, in add_metadata
    ds_out['xc'] = ds_out['xc'].assign_attrs({'long_name':'longitude of grid cell center'})
NameError: name 'ds_out' is not defined

Of course it's highly likely to be user error

rljacob commented 3 weeks ago

I agree with @mahf708. Move the docs directory to just under tools.

whannah1 commented 3 weeks ago

After discussing how to restructure the tools docs with @rljacob we didn't like the alternative, so we decided to leave it as is.

I also fixed the bug that @jonbob found.

whannah1 commented 3 weeks ago

@singhbalwinder looks like I have another problem with the output domain files that I need to investigate, so we need to hold off on merging this.

mahf708 commented 3 weeks ago

looks like I have another problem with the output domain files that I need to investigate, so we need to hold off on merging this.

@whannah1, not to add more work to this, but should we consider adding a unit test to go with this? Then, we can even have that unit test go in one of these actions. An example of a python-centric unit test:

If we have these files on the inputdata server, it is then relatively easy to request them, etc. (as long as we are not accumulating more than ~20 GB or so, we should be able to test this pretty quickly).

I thought it would be opportune time to suggest since you're likely debugging and having a clear systematic view of the code, etc., and I am happy to put together whatever you end up assembling in a gh-ci-compliant format :)

whannah1 commented 3 weeks ago

@mahf708 I want to hold off on the testing idea mainly because I'm not sure how to write a coherent test. We could set it up to just go through motions and prove that it can produce a set of files, but I'm not sure how to test the output for any sort of correctness. So let's hold off.

@singhbalwinder The issue that Jon identified is now fixed, so this PR is ready to be merged.

whannah1 commented 3 weeks ago

As one last test I ran a quick test using F2010 on the ne30pg2_oECv3 grid replacing the default domain files with ones produced by the new tool and it ran without issue. However, I didn't do a baseline comparison.

singhbalwinder commented 2 weeks ago

Merged to next