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

Pace util mostly does not depend on dsl #93

Merged
merged 2 commits into from
Jan 11, 2022
Merged

Conversation

elynnwu
Copy link
Collaborator

@elynnwu elynnwu commented Jan 11, 2022

Purpose

Part I of pace.util does not depend on pace.dsl. In this PR, we remove dependencies of make_storage in DriverGridData. Additional edge vector data are added in Metric Terms so that we can directly access their storage in DriverGridData. We decide to allow pace.util to still depend on pace.dsl.stencil.GridIndexing (used strictly in fill corners in Metric Terms).

Code changes:

  • Provide a list of relevant code changes and the side effects they have on other code.

Requirements changes:

  • Provide a list of any changes made to requirements, e.g. changes to files requirements*.txt, constraints.txt, setup.py, pyproject.toml, pre-commit-config.yaml and a reason if not included in the Purpose section (e.g. incompatibility, updates, etc)

Infrastructure changes:

  • Added edge_vect_e_2d and edge_vect_w_2d in Metric Terms: they are the x/y repeated version of edge_vect_e/w
  • Fixed fv_update_phys reference to dycore state quantity
  • Changed test_driver_runs to also run post physics code

Checklist

Before submitting this PR, please make sure:

  • You have followed the coding standards guidelines established at Code Review Checklist.
  • Docstrings and type hints are added to new and updated routines, as appropriate
  • All relevant documentation has been updated or added (e.g. README, CONTRIBUTING docs)

Copy link
Contributor

@rheacangeo rheacangeo left a comment

Choose a reason for hiding this comment

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

Great job!

Comment on lines -379 to 378
state.u.synchronize()
state.v.synchronize()
state.ua.synchronize()
state.va.synchronize()
Copy link
Contributor

Choose a reason for hiding this comment

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

do you know why we do or do not need the synchronize for u vs ua?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this one is because we're not using slice_output, which calls synchronize with certain backend. Since here the output is prepared manually, we call synchronize explicitly for the variables that were halo exchanged.

Comment on lines +2206 to +2209
edge_vect_e_2d.data[:-1, :-1], edge_vect_w_2d.data[:-1, :-1] = (
east_edge_data[:-1, :-1],
west_edge_data[:-1, :-1],
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this initially made me worried, assigning values to existing quantities with the gpu backend has been finnicky in various situations, and this code won't get exercised on the gpu at the moment. But I see other methods here using this pattern on code that is used in the dycore and thus is tested on GPU, so looks good.

@@ -1,9 +1,6 @@
import dataclasses
from typing import Any, Optional

import numpy as np

from pace.dsl import gt4py_utils as utils
Copy link
Contributor

Choose a reason for hiding this comment

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

yay for less cycles!

@elynnwu elynnwu merged commit 343926c into main Jan 11, 2022
@elynnwu elynnwu deleted the fix/remove-circular-dep-part1 branch January 11, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants