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.

Sign up for GitHub

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

Timeloop #308

Merged
merged 30 commits into from
Nov 30, 2023
Merged

Timeloop #308

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c164b13
Constructing time loop
OngChia Oct 9, 2023
0f22f54
Constructing time loop
OngChia Oct 9, 2023
1a4a7a8
COnstructing time loop test
OngChia Oct 9, 2023
f84cae2
Merge branch 'main' into timeloop
OngChia Oct 11, 2023
80e4a62
Constructing timeloop
OngChia Oct 12, 2023
9871226
Constructing time loop
OngChia Oct 13, 2023
44dcf11
time loop under construction
OngChia Oct 18, 2023
9b6c241
Created a test for timeloop
OngChia Oct 20, 2023
aa34553
Updated the time_step call in timeloop
OngChia Oct 20, 2023
139ebb0
Finalizing timeloop and its test
OngChia Nov 6, 2023
f4924f5
Finalized timeloop before attempting to merge
OngChia Nov 7, 2023
46dd017
Resolve conflicts
OngChia Nov 7, 2023
d81909f
Resolving pre-commit failed runs
OngChia Nov 7, 2023
fa46f6c
Attempting to resolve shadowing of Python builtin in stencil 14
OngChia Nov 7, 2023
368cc57
Resolving A004 error code for abs import in dycore velocity_stencil 1…
OngChia Nov 7, 2023
fc315a1
Removed adding A004 to the flake8 ignore list in dycore
OngChia Nov 8, 2023
e86bb81
Resolving conflicts
OngChia Nov 8, 2023
df63714
Fixed bugs due to recent update of grid infrastructure, and add secon…
OngChia Nov 8, 2023
510f4dd
CLeaning up timeloop
OngChia Nov 8, 2023
9eae2c1
Resolved conflicts
OngChia Nov 8, 2023
0d4dddd
Passed qa after resolving conflicts
OngChia Nov 8, 2023
a85e3ce
Resolving issues brought up in review
OngChia Nov 17, 2023
4c3209e
Removed a bug in r04b09_iconrun_config where appla_initial_stabilizat…
OngChia Nov 17, 2023
25ab752
Merge branch 'main' of https://github.com/C2SM/icon4py into timeloop
OngChia Nov 20, 2023
4eab095
Made changes according to second review
OngChia Nov 22, 2023
c6461be
Made changes according to icon4py qa
OngChia Nov 22, 2023
56b992f
Added TODO of diagnostic variable preparation in time_integration nec…
OngChia Nov 22, 2023
664c80a
Update README.md of driver and change np.asarray to .asnumpy()
OngChia Nov 28, 2023
066bd0b
Resolved conflicts with main branch, added tests for the second time …
OngChia Nov 29, 2023
9485e34
Resolved conflicts
OngChia Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/nh_solve/solve_nonhydro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ def time_step(
diagnostic_state_nh: DiagnosticStateNonHydro,
prognostic_state_ls: list[PrognosticState],
prep_adv: PrepAdvection,
z_fields: ZFields,
z_fields: ZFields, # TODO (Magdalena): move local fields to SolveNonHydro
nh_constants: NHConstants,
bdy_divdamp: Field[[KDim], float],
dtime: float,
Expand Down Expand Up @@ -556,24 +556,23 @@ def run_predictor_step(
nnow: int,
nnew: int,
):
# TODO (magdalena) fix this! when fixing call of velocity advection in predictor, condition is broken,
if l_init or l_recompute:
if self.config.itime_scheme == 4 and not l_init:
lvn_only = True # Recompute only vn tendency
else:
lvn_only = False

Copy link
Contributor

Choose a reason for hiding this comment

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

I still believe this if-condition is very obfuscated. (I know you did not change it...) but I always wondered what is the value of the lvn_only if l_init==False and l_recompute == False? I think it is not defined, and that is probably just ok because for some reason it never happens.

Furthermore isn't this the same:

Suggested change
if l_init:
lvn_only = False
else if l_recompute and self.config.itime_scheme == 4:
lvn_only = True

Copy link
Contributor Author

@OngChia OngChia Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From my understanding, the outer if (l_init or l_recompute)) determines whether velocity_advection.run_predictor_step will be run. And lvn_only is only needed in velocity_advection.run_predictor_step. So, when l_init==False and l_recompute==False, the value of lvn_only does not matter. I remember Anurag said that we will possibly switch to itime_scheme=6 in the future and we can completely remove this weird lvn_only. I suggest we just leave it as the current state as a temporary solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, still don't like it. The condition is just very hard to read.

self.velocity_advection.run_predictor_step(
vn_only=lvn_only,
diagnostic_state=diagnostic_state_nh,
prognostic_state=prognostic_state[nnow],
z_w_concorr_me=self.z_w_concorr_me,
z_kin_hor_e=z_fields.z_kin_hor_e,
z_vt_ie=z_fields.z_vt_ie,
dtime=dtime,
ntnd=self.ntl1,
cell_areas=self.cell_areas,
)
self.velocity_advection.run_predictor_step(
vn_only=lvn_only,
diagnostic_state=diagnostic_state_nh,
prognostic_state=prognostic_state[nnow],
z_w_concorr_me=self.z_w_concorr_me,
z_kin_hor_e=z_fields.z_kin_hor_e,
z_vt_ie=z_fields.z_vt_ie,
dtime=dtime,
ntnd=self.ntl1,
cell_areas=self.cell_areas,
)

p_dthalf = 0.5 * dtime

Expand Down Expand Up @@ -1698,7 +1697,8 @@ def run_corrector_step(
offset_provider={},
)

if not self.config.l_open_ubc and not self.l_vert_nested:
# TODO (magdalena): delete NonHydrostaticConfig. l_open_ubc
if not (self.config.l_open_ubc and self.l_vert_nested):
halungge marked this conversation as resolved.
Show resolved Hide resolved
mo_solve_nonhydro_stencil_46.with_backend(run_gtfn)(
w_nnew=prognostic_state[nnew].w,
z_contr_w_fl_l=z_fields.z_contr_w_fl_l,
Expand Down
79 changes: 40 additions & 39 deletions model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/velocity/velocity_advection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,46 +346,47 @@ def run_predictor_step(
offset_provider={"Koff": KDim},
)

velocity_prog.fused_stencils_16_to_17.with_backend(run_gtfn)(
w=prognostic_state.w,
local_z_v_grad_w=self.z_v_grad_w,
e_bln_c_s=self.interpolation_state.e_bln_c_s,
local_z_w_con_c=self.z_w_con_c,
coeff1_dwdz=self.metric_state.coeff1_dwdz,
coeff2_dwdz=self.metric_state.coeff2_dwdz,
ddt_w_adv=diagnostic_state.ddt_w_adv_pc[ntnd],
horizontal_start=start_cell_nudging,
horizontal_end=end_cell_local,
vertical_start=1,
vertical_end=self.grid.num_levels,
offset_provider={
"C2E": self.grid.get_offset_provider("C2E"),
"C2CE": self.grid.get_offset_provider("C2CE"),
"Koff": KDim,
},
)
if not vn_only:
velocity_prog.fused_stencils_16_to_17.with_backend(run_gtfn)(
w=prognostic_state.w,
local_z_v_grad_w=self.z_v_grad_w,
e_bln_c_s=self.interpolation_state.e_bln_c_s,
local_z_w_con_c=self.z_w_con_c,
coeff1_dwdz=self.metric_state.coeff1_dwdz,
coeff2_dwdz=self.metric_state.coeff2_dwdz,
ddt_w_adv=diagnostic_state.ddt_w_adv_pc[ntnd],
horizontal_start=start_cell_nudging,
horizontal_end=end_cell_local,
vertical_start=1,
vertical_end=self.grid.num_levels,
offset_provider={
"C2E": self.grid.get_offset_provider("C2E"),
"C2CE": self.grid.get_offset_provider("C2CE"),
"Koff": KDim,
},
)

mo_velocity_advection_stencil_18.with_backend(run_gtfn)(
levmask=self.levmask,
cfl_clipping=self.cfl_clipping,
owner_mask=self.c_owner_mask,
z_w_con_c=self.z_w_con_c,
ddqz_z_half=self.metric_state.ddqz_z_half,
area=cell_areas,
geofac_n2s=self.interpolation_state.geofac_n2s,
w=prognostic_state.w,
ddt_w_adv=diagnostic_state.ddt_w_adv_pc[ntnd],
scalfac_exdiff=scalfac_exdiff,
cfl_w_limit=cfl_w_limit,
dtime=dtime,
horizontal_start=start_cell_nudging,
horizontal_end=end_cell_local,
vertical_start=int32(max(3, self.vertical_params.index_of_damping_layer - 2) - 1),
vertical_end=int32(self.grid.num_levels - 3),
offset_provider={
"C2E2CO": self.grid.get_offset_provider("C2E2CO"),
},
)
mo_velocity_advection_stencil_18.with_backend(run_gtfn)(
levmask=self.levmask,
cfl_clipping=self.cfl_clipping,
owner_mask=self.c_owner_mask,
z_w_con_c=self.z_w_con_c,
ddqz_z_half=self.metric_state.ddqz_z_half,
area=cell_areas,
geofac_n2s=self.interpolation_state.geofac_n2s,
w=prognostic_state.w,
ddt_w_adv=diagnostic_state.ddt_w_adv_pc[ntnd],
scalfac_exdiff=scalfac_exdiff,
cfl_w_limit=cfl_w_limit,
dtime=dtime,
horizontal_start=start_cell_nudging,
horizontal_end=end_cell_local,
vertical_start=int32(max(3, self.vertical_params.index_of_damping_layer - 2) - 1),
vertical_end=int32(self.grid.num_levels - 3),
offset_provider={
"C2E2CO": self.grid.get_offset_provider("C2E2CO"),
},
)

# This behaviour needs to change for multiple blocks
self.levelmask = self.levmask
Expand Down
Loading