-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature/fv3core fortran api #395
Conversation
from pace import fv3core | ||
|
||
|
||
class GeosDycoreWrapper: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] I believe NASA always refers it as GEOS
|
||
return self.output_dictionary | ||
|
||
def _put_fortran_data_in_dycore( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to check - is the idea here to not use translate fvdynamics anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that's the idea
}, | ||
"initialization": {"type": "baroclinic"}, | ||
"nx_tile": 12, | ||
"nz": 79, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want to test the additional vertical case you added, either 91 or 72
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so
@@ -204,14 +204,372 @@ def set_hybrid_pressure_coefficients(km: int) -> HybridPressureCoefficients: | |||
1, | |||
] | |||
) | |||
|
|||
elif km == 91: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine since this is how we set up our npz=79 case, but we should probably merge this so it is possible to replace ak/bk with custom values.
class GeosDycoreWrapper: | ||
""" | ||
Provides an interface for the Geos model to access the Pace dycore. | ||
Takes numpy arrays as inputs, returns a dictionary of numpy arrays as outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this only works for numpy and cpu backend? We should extend this to take cupy array as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we can also make this work on GPU. The reason for the current input/output setup is to comport with Purnendu's code, which puts the Fortran data in numpy arrays and reads a dictionary back out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be gpu-functional now, I hope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be safe. Probably you want to had some comments in there to explain the upload mechanism.
Fortran comes as CPU
Dycore state is init two a GPU storage thanks to the backend
in QuantityFactory
safe_assign_array
is capable of recognizing that source & destination are not in the same memory space and deals with the upload / download.
In case Fortran memory is on GPU (unlikely) and comes as a cupy
array this will take a copy
This upload mechanism is slow and requires it's own optimization via staggered cudaMemCpyAsync
. This is not present at the moment
if "fv_core_nml" in namelist.keys(): | ||
layout = namelist["fv_core_nml"]["layout"] | ||
# npx and npy in the namelist are cell centers, but npz is mid levels | ||
nx_tile = namelist["fv_core_nml"]["npx"] - 1 | ||
ny_tile = namelist["fv_core_nml"]["npy"] - 1 | ||
nz = namelist["fv_core_nml"]["npz"] | ||
elif "nx_tile" in namelist.keys(): | ||
layout = namelist["layout"] | ||
# everything is cell centered in this format | ||
nx_tile = namelist["nx_tile"] | ||
ny_tile = namelist["nx_tile"] | ||
nz = namelist["nz"] | ||
return cls.from_tile_params( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning: these seems to be no else
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be ok
…y allocation to __init__
Co-authored-by: Johann Dahm <[email protected]>
…1 vertical levels
…m/pace into feature/fv3core_fortran_api dont edit in browser
diss_estd: np.ndarray, | ||
) -> dict: | ||
|
||
self._put_fortran_data_in_dycore( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grab the timer from pace.util
and slap one around each of those function call. See fv_dynamics
for usage. They are GPU robust already
launch jenkins |
@@ -114,38 +115,41 @@ def __call__( | |||
diss_estd: np.ndarray, | |||
) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not making this more strongly typed? Could it be Dict[str, np.ndarray]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small oversight, thought I'd done that
launch jenkins |
Purpose
This PR adds an API to call the Pace dycore from GEOS.
Code changes:
from_namelist
method to work with our updated namelist configuration that doesn't usefv_core_nml
as a key.Checklist
Before submitting this PR, please make sure:
pace-util
, HISTORY has been updated