D-grid data input method #42
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge branch 'main' into external_grid
…grid. Current method uses xarray to interact with netcdf tile files. Values for longitutde, latitude, number of points in x an y, grid edge distances read in.
fmalatino
changed the title
fmalatino
force-pushed
the
external_grid
branch
from
4041833 to
c6fe0eb
Compare
FlorianDeconinck
requested changes
Dec 8, 2023
| type: external | ||
| config: | ||
| grid_type: 0 | ||
| grid_file_path: "../../test_input/C12.tile" |
There was a problem hiding this comment.
This not part of the PR. This wasn't picked up by the tests because the 54 ranks tests are not running right now!
There was a problem hiding this comment.
Is there way I can include them? Should we be running the 54 rank tests?
There was a problem hiding this comment.
How big are the files? Technically I think it should be located at driver/examples/data so that the example folder is indeed self standing.
We should be running 54 ranks but I haven't look at if we can do that on the Github free runners without it taking 8 million years. Someone has to go ahead and make a github action and test it out
There was a problem hiding this comment.
We can start a plan to set up a CI which uses cloud resources from the NOAA cloud resources provider. We've done this successfully for SHiELD, FMS, and other GFDL repos. The key is to make sure we have a container with the needed bits.
| type: external | ||
| config: | ||
| grid_type: 0 | ||
| grid_file_path: "../../test_input/C12.tile" |
There was a problem hiding this comment.
Since these are small files, they should be okay. Ideally we'd store them in the container (mentioned above) or generate them on the fly to test the grid creation and ingest as a group.
Co-authored-by: Florian Deconinck <[email protected]>
bensonr
reviewed
Dec 11, 2023
…and class descriptor
|
Hi,
Yes, that is correct. Each of the terms are initialized as None types at
the creation of a metric terms object and are given values at the time of
their call if not already specified (as in the lon and lat types read in
from this method, or when the dx and dy are set to constant values, like
when specifying dx_const for a Cartesian type grid).
…On Mon, Dec 18, 2023 at 3:01 PM lharris4 ***@***.***> wrote:
Hi, Frank. Thank you for your explanation. So in that case, the metrics
are
not actually calculated until the property methods of each metric term
datatype is called?
Lucas
On Mon, Dec 18, 2023 at 11:50 AM fmalatino ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In tests/mpi_54rank/test_external_grid_1x1.py
> <#42 (comment)>:
>
> > +
> + subtile_slice_dy = cube_comm.partitioner.tile.subtile_slice(
> + rank=cube_comm.rank,
> + global_dims=[pace.util.Y_DIM, pace.util.X_INTERFACE_DIM],
> + global_extent=(ny, npx),
> + overlap=True,
> + )
> +
> + subtile_slice_area = cube_comm.partitioner.tile.subtile_slice(
> + rank=cube_comm.rank,
> + global_dims=[pace.util.Y_DIM, pace.util.X_DIM],
> + global_extent=(ny, nx),
> + overlap=True,
> + )
> +
> + lon_rad = lon * (PI / 180)
>
> @lharris4 <https://github.com/lharris4> To partially address your
> question regarding the read-in and computation of metric terms, the
> open_dataset() method from xarray is used to read data, from a netcdf
file
> type, specifically the lon and lat points from a grid data file. These
> points are then used to set the lon and lat points for the metric terms
> object generated at the time of the creation of a driver object by a PE.
> The remaining metric terms are then calculated, when called upon, by the
> "property" methods of the metric terms class, using the read in lon and
lat
> points. If not using the read-in method, the lon and lat points will be
> generated by what is stated in the configuration yaml. I hope I didn't
make
> this more confusing.
>
> —
> Reply to this email directly, view it on GitHub
> <#42 (comment)>, or
> unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AMUQRVEUHPTAE7VOXADZTE3YKBX5XAVCNFSM6AAAAABAIDXLEOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOBXGMYTQNRRGE>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#42 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/BB6BH6SD2B4RSKMDZ6WXMULYKCOKFAVCNFSM6AAAAABAIDXLEOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRRGQ4TINZZGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Frank Malatino
Scientific Programmer
+1 609.452.5066 (work)
+1 609.661.4520 (cell)
SAIC/NOAA/GFDL
|
oelbert
reviewed
Jan 3, 2024
|
|
||
|
|
||
| def get_tile_num(comm: MPIComm): | ||
| return pace.util.get_tile_index(comm.rank, comm.partitioner.total_ranks) |
There was a problem hiding this comment.
If you use pace.util.get_tile_number here you don't have to add 1 later
There was a problem hiding this comment.
Ok will use that instead
|
|
||
| if mpicomm.Get_rank() == 0: | ||
| total_area = math.fsum(tile_area) | ||
| print(diffs) |
There was a problem hiding this comment.
does this mean you're only printing the diffs on rank 0?
There was a problem hiding this comment.
Ah yes, rank 0 would be the only one printing its diffs, I will change this so that all ranks print their relative errors.
| @@ -0,0 +1,156 @@ | |||
| import math | |||
There was a problem hiding this comment.
Same comments as on the 1x1 layout
| type: external | ||
| config: | ||
| grid_type: 0 | ||
| grid_file_path: "../../test_input/C12.tile" |
There was a problem hiding this comment.
@FlorianDeconinck What do you think about including the tile data file in the PR? I know we don't love having test data in the repo but this one is small and if we want the tests run as part of CI...
…each rank and get_tile_number replacing get_tile_index
oelbert
approved these changes
Jan 4, 2024
|
Also you should add yourself to the contributors list! |
Co-authored-by: Oliver Elbert <[email protected]>
bensonr
reviewed
Jan 24, 2024
| @@ -0,0 +1,155 @@ | |||
| import math | |||
There was a problem hiding this comment.
Is there a way to combine the 1x1 and 2x2 test into a single python test accepting an argument - or is that not the convention we are adopting in Pace?
…ta locations from command line, updated test description, and added documentation on grid construction to external grid data configuration selection dataclass.
…ionality of pytest
bensonr
previously approved these changes
Jan 30, 2024
bensonr
approved these changes
Jan 30, 2024
bensonr
previously approved these changes
Jan 30, 2024
bensonr
approved these changes
Jan 30, 2024
fmalatino
added a commit
that referenced
this pull request
Jan 30, 2024
* Benchmark changes * Added benchmark configurations * Removed benchmark configs from configs to be tested in unit tests * Changed dace submodule to point to Florian fix * Dace submodule points to gcc_dies_on_dacecpu branch * Set 'rf_fast' to true in baroclinic_c384_cpu/gpu.yaml * fix mpi4py version (#51) * [Feature] Better translate test (#39) (#47) Translate test: small improvements - Parametrize perturbation upon failure - Refactor error folder to be `pwd` based - Fix GPU translate unable to dump error `nc` - Fix mixmatch precision on translate test - Update README.md Test fix: - Orchestrate YPPM for translate purposes Misc: - Fix bad logger formatting on DaCeProgress * [NASA] [Feature] Guarding against unimplemented configuration (#40) (#48) * [Feature] Guarding against unimplemented configuration (#40) Guarding against unimplemented namelists options: - a2b_ord4 - d_sw - fv_dynamics - fv_subgridz - neg_adj3 - divergence damping - xppm - yppm Misc: - Fix `netcdf_monitor` not mkdir the directory - Add `as_dict` to the dycore state to dump the dycore more easily * Unused assert * Update fv3core/pace/fv3core/stencils/yppm.py Co-authored-by: Oliver Elbert <[email protected]> * Update fv3core/pace/fv3core/stencils/xppm.py Co-authored-by: Oliver Elbert <[email protected]> * Change NotImplemented to ValueError for n_sponge<3 * lint --------- Co-authored-by: Oliver Elbert <[email protected]> * Re-try of updating dace submodule to track Florian fix branch * Reverted gt4py submodule to match main checkout * Added benchmark README file * Read in ak, bk coefficients (#36) * initial changes to read in ak bk * read ak/bk * add xfail * remove input dir * further changes to unit tests * finish up test * add history * commit uncommited files * fix test comment * add input to top * read in data * read in netcdf file in eta mod * remove txt file * test * modify test and fix generate.py * remove emacs backup file * driver tests pass * fix helper.py * fix fv3core tests * fix physics test * fix grid tests * nullcommconfig * cleanup input * remove driver input * remove top level input * fix circular import problems * modify eta_file readin for test_restart_serial * comment out 91 test * rm safety checks * revert diagnostics.py * restore driver.py * revert initialization.py * restore state.py * restore analytic_init.py * restore init_utils.py and analytic_init.py * restore c_sw.py * d2a2c_vect.py * restore fv3core/stensils * restore translate_fvdynamics * restore physics/stencils * restore stencils * remove circular dependency * use pytest parametrize * cleanup generation.py * fstrinngs * add eta_file to MetricTerm init * remove eta_file argument in new_from_metric_terms and geos_wrapper * use pytest parametrize for the xfail tests * use pytest parametrize for the xfail tests * fix geos_wrapper and grid * fix tests * fstring * add test comments * fix util/HISTORY.md * fix comments * remove __init__.py from tests/main/grid * add jupyter notebooks to generate eta files * generate ak,bk,ptop on metricterm init * fix tests * exploit np.all in eta mod * remove tests/main/grid/input * update ci * test * remove input * edit ci yaml * remove push --------- Co-authored-by: mlee03 <[email protected]> * Move Active Physics Schemes to Config (#44) * initial commit, need to adapt and run tests * revising scheme name * tests pass * update history * linting * changing typehints for physics schemes to enum instead of str * driver now works with physics config enum, tests pass * fixed tests * missed one * D-grid data input method (#42) * Testing changes reflected across branches * Undoing changes made in build_gaea_c5.sh * Testing vscode functionality, by adding a change to external_grid branch * Testing vscode functionality, by adding a change to external_grid branch * Addition of from_generated method and calc_flag to util/pace/util/grid/generation.py * Added get_grid method for external grid data to driver/pace/driver/grid.py * Preliminary xarray netcdf read in method added to driver/pace/driver/grid.py * Updating util/pace/util/grid/generation.py from_generated method * Addition of external grid data read in methods for initialization of grid. Current method uses xarray to interact with netcdf tile files. Values for longitutde, latitude, number of points in x an y, grid edge distances read in. * driver/examples/configs/test_external_C12_1x1.yaml * Preliminary unit test for external grid data read in * Current state of unit tests as of 27 Nov 2023 * External grid method and unit tests added * Re-excluding external grid data yamls from test_example_configs.py * Update driver/pace/driver/grid.py Co-authored-by: Florian Deconinck <[email protected]> * Changed name of grid initializer function to match NetCDF dependency and class descriptor * Update util/pace/util/grid/generation.py Moved position of doc string for "from_external" MetricTerms class method Co-authored-by: Oliver Elbert <[email protected]> * Fixed indentation error in generation.py from suggestion in PR 42 * Removal of TODO comment in grid.py, changes to method of file accessing in test_analytic_init, test_external_grid_* * Changed grid data read-in unit tests to compare data directly from file to driver grid data generated from yaml * Change to reading in lon and lat, other metric terms calculated as needed * Removed read-in of dx, dy, and area. Changed unit tests to compare calculated area to 'ideal' surface area as given by selected constants type. * Update tests/mpi_54rank/test_external_grid_1x1.py Incorrect name of test in test_external_grid_1x1.py changed to match file name Co-authored-by: Oliver Elbert <[email protected]> * Added comparisons for read-in vs generated by driver lon, lat, dx, dy, and area data to unit tests * Added relative error calculations to unit tests for external grid data read-in * External grid data read in tests changed: relative errors printed by each rank and get_tile_number replacing get_tile_index * Removing commented out sections in test_external_grid_2x2.py Co-authored-by: Oliver Elbert <[email protected]> * Updated external grid data read-in to take configuration and input data locations from command line, updated test description, and added documentation on grid construction to external grid data configuration selection dataclass. * Updated documentation in grid.py * Updated external grid data read in unit test to use parametrize functionality of pytest * Ammended files to reference changes to PR 36 --------- Co-authored-by: Frank Malatino <[email protected]> Co-authored-by: Florian Deconinck <[email protected]> Co-authored-by: Oliver Elbert <[email protected]> --------- Co-authored-by: Oliver Elbert <[email protected]> Co-authored-by: Florian Deconinck <[email protected]> Co-authored-by: Oliver Elbert <[email protected]> Co-authored-by: MiKyung Lee <[email protected]> Co-authored-by: mlee03 <[email protected]> Co-authored-by: Frank Malatino <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Implementation of methods to allow for grid data from NetCDF (as generated by FRE-NCtools) to be read-in and used to initialize metric terms for the creation of a driver object.
The user can now specify the path for grid data files to be read-in in the configuration yaml file. Each rank will select from the files their appropriate slice of data using the subtile_slice methods included in Pace.
Code changes:
Requirements changes:
N/A
Infrastructure changes:
N/A
Checklist
Before submitting this PR, please make sure:
pace-util, HISTORY has been updatedAdditionally, if this PR contains code authored by new contributors: