Skip to content

Commit

Permalink
merge with upstream
Browse files Browse the repository at this point in the history
  • Loading branch information
nfarabullini committed May 29, 2024
2 parents c1a6343 + a2e6849 commit 70223e1
Show file tree
Hide file tree
Showing 211 changed files with 1,348 additions and 1,802 deletions.
168 changes: 168 additions & 0 deletions CODING_GUIDELINES.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
# Coding Guidelines

## Software design

Designing a clean architecture for a software product is a difficult task and developers can easily overlook it under pressure or when they are eager to deliver exciting new features. However, choosing the quick solution instead of a clean one will likely lead to a large amount of additional work in the future (_technical debt_).

To keep technical debt at acceptable levels, design and implementation of new features should follow best practices:

1. Make sure your code is always covered by automatic testing to ensure its correctness. Where unit tests are impractical, use integration tests.
2. Adhere to the [SOLID](https://en.wikipedia.org/wiki/SOLID) principles of software design.
3. Do not reinvent the wheel: if someone solved your problem within the project or in a third party library, consider using their solution or extending it before writing a new component for the same purpose.
4. _You ain't gonna need it_ ([YAGNI](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it)): do not design solutions for problems that might come up in the future, as chances are that you will never need that code. Focus on current problems and prepare for future requirements by writing clean code.
5. _Do not repeat yourself_ ([DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself)): if you are writing the same code snippet in several places, extract it into a function.
6. Use meaningful names: the purpose of an object should be clear from its name. Usually, class names are nouns and function names are verbs.

<!--
TODO: we should add a doc folder (separate for `tools` and `model`? separate for each package?)
Remember that important design decisions should be properly documented for future reference and to share the knowledge with other developers. We decided to use lightweight _Architecture Decision Records_ (ADRs) for this purpose. The full list of ADRs and documentation for writing new ones can be found in [docs/functional/architecture/Index.md](docs/functional/architecture/Index.md).
-->
## Code Style

We follow the [Google Python Style Guide][google-style-guide] with a few minor changes (mentioned below). Since the best way to remember something is to understand the reasons behind it, make sure you go through the style guide at least once, paying special attention to the discussions in the _Pros_, _Cons_, and _Decision_ subsections.

We deviate from the [Google Python Style Guide][google-style-guide] only in the following points:

- We use [`ruff-linter`][ruff-linter] instead of [`pylint`][pylint].
- We use [`ruff-formatter`][ruff-formatter] for source code and imports formatting, which may work differently than indicated by the guidelines in section [_3. Python Style Rules_](https://google.github.io/styleguide/pyguide.html#3-python-style-rules). For example, maximum line length is set to 100 instead of 79 (although docstring lines should still be limited to 79).
- According to subsection [_2.19 Power Features_](https://google.github.io/styleguide/pyguide.html#219-power-features), direct use of _power features_ (e.g. custom metaclasses, import hacks, reflection) should be avoided, but standard library classes that internally use these power features are accepted. Following the same spirit, we allow the use of power features in infrastructure code with similar functionality and scope as the Python standard library.
- According to subsection [_3.19.12 Imports For Typing_](https://google.github.io/styleguide/pyguide.html#31912-imports-for-typing), symbols from `typing` and `collections.abc` modules used in type annotations _"can be imported directly to keep common annotations concise and match standard typing practices"_. Following the same spirit, we allow symbols to be imported directly from third-party or internal modules when they only contain a collection of frequently used typying definitions.

Further guidance and repository- or package specific guidelines can be found in the respective `docs` folders and `README.md` files.
In particular
- [naming conventions for stencils](model/docs/stencil_naming_convention.md)
- [liskov integration style guide ](tools/docs/ICON_Liskov_integration_style_guide.md)

### Common questions

- `pass` vs `...` (`Ellipsis`)

`pass` is the _no-op_ statement in Python and `...` is a literal value (called _Ellipsis_) introduced for slicing collections of unknown number of dimensions. Although they are very different in nature, both of them are used in places where a statement is required purely for syntactic reasons, and there is not yet a clear standard practice in the community about when to use one or the other. We decided to align with the common pattern of using `...` in the body of empty function definitions working as placeholders for actual implementations defined somewhere else (e.g. type stubs, abstract methods and methods appearing in `Protocol` classes) and `pass` in any other place where its usage is mixed with actual statements.

```python
# Correct use of `...` as the empty body of an abstract method
class AbstractFoo:
@abstractmethod
def bar(self) -> Bar:
...

# Correct use of `pass` when mixed with other statements
try:
resource.load(id=42)
except ResourceException:
pass
```


### Error messages

Error messages should be written as sentences, starting with a capital letter and ending with a period (avoid exclamation marks). Try to be informative without being verbose.
The message should be kept to one sentence if reasonably possible.
Code objects such as 'ClassNames' and 'function_names' should be enclosed in single quotes, and so should string values used for message interpolation.

Examples:

```python
raise ValueError(f"Invalid argument 'dimension': should be of type 'Dimension', got '{dimension.type}'.")
```

Interpolated integer values do not need double quotes, if they are indicating an amount. Example:

```python
raise ValueError(f"Invalid number of arguments: expected 3 arguments, got {len(args)}.")
```

The double quotes can also be dropped when presenting a sequence of values. In this case the message should be rephrased so the sequence is separated from the text by a colon ':'.

```python
raise ValueError(f"unexpected keyword arguments: {', '.join(set(kwarg_names} - set(expected_kwarg_names)))}.")
```

### Docstrings
We encourage to add doc strings for functions, classes and modules if they help the reader understand the code and contain information that is not obvious from the code itself. While we do not yet generate API documentation from doc strings we might do so in the future using [Sphinx][sphinx] and some extensions such as [Sphinx-autodoc][sphinx-autodoc] and [Sphinx-napoleon][sphinx-napoleon]. These follow the Google Python Style Guide docstring conventions to automatically format the generated documentation. A complete overview can be found here: [Example Google Style Python Docstrings](https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html#example-google).

Sphinx supports the [reStructuredText][sphinx-rest] (reST) markup language for defining additional formatting options in the generated documentation, however section [_3.8 Comments and Docstrings_](https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings) of the Google Python Style Guide does not specify how to use markups in docstrings. As a result, we decided to forbid reST markup in docstrings, except for the following cases:

- Cross-referencing other objects using Sphinx text roles for the [Python domain](https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#the-python-domain) (as explained [here](https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#python-roles)).
- Very basic formatting markup to improve _readability_ of the generated documentation without obscuring the source docstring (e.g. ` ``literal`` ` strings, bulleted lists).

We highly encourage the [doctest][doctest] format for code examples in docstrings. In fact, doctest runs code examples and makes sure they are in sync with the codebase.

### Module structure

In general, you should structure new Python modules in the following way:

1. _shebang_ line: `#! /usr/bin/env python3` (only for **executable scripts**!).
2. License header (see `.license_header.txt`, it is added automatically by `pre-commit` hook)
3. Module docstring.
4. Imports, alphabetically ordered within each block (fixed automatically by `ruff-formatter`):
1. Block of imports from the standard library.
2. Block of imports from general third party libraries using standard shortcuts when customary (e.g. `numpy as np`).
3. Block of imports from specific modules of the project.
5. Definition of exported symbols (optional, mainly for re-exporting symbols from other modules):

```python
__all__ = ["func_a", "CONST_B"]
```

6. Public constants and typing definitions.
7. Module contents organized in a convenient way for understanding how the pieces of code fit together, usually defining functions before classes.

Try to keep sections and items logically ordered, add section separator comments to make section boundaries explicit when needed. If there is not a single evident logical order, pick the order you consider best or use alphabetical order.

Consider configuration files as another type of source code and apply the same criteria, using comments when possible for better readability.

### Ignoring QA errors

You may occasionally need to disable checks from _quality assurance_ (QA) tools (e.g. linters, type checkers, etc.) on specific lines as some tool might not be able to fully understand why a certain piece of code is needed. This is usually done with special comments, e.g. `# noqa: F401`, `# type: ignore`. However, you should **only** ignore QA errors when you fully understand their source and rewriting your code to pass QA checks would make it less readable. Additionally, you should add a short descriptive code if possible (check [ruff rules][ruff-rules] and [mypy error codes][mypy-error-codes] for reference):

```python
f = lambda: 'empty' # noqa: E731 [lambda-assignment]
```

and, if needed, a brief comment for future reference:

```python
...
return undeclared_symbol # noqa: F821 [undefined-name] on purpose to trigger black-magic
```

## Testing

Testing components is a critical part of a software development project. We follow standard practices in software development and write unit, integration, and regression tests. Note that even though [doctests][doctest] are great for documentation purposes, they lack many features and are difficult to debug. Hence, they should not be used as replacement for proper unit tests except in trivial cases.

### Test suite folder structure
In each package tests are organized under the `tests` folder. The `tests` folder should not be a package but the contained test suites may be python packages.

Test suites in folders `stencil_tests` are generally run in integration mode with [icon-exclaim](https://github.com/C2SM/icon-exclaim) and should only contain tests for the GT4Py stencils that might be integrated into ICON.


## Further


<!--
TODO: add test conventions:
TODO: - to name test functions
TODO: - to use pytest features (fixtures, markers, etc.)
TODO: - to generate mock objects and data for tests (e.g. pytest-factoryboy, pytest-cases)
TODO: - to use pytest plugins

-->

<!-- Reference links -->

[doctest]: https://docs.python.org/3/library/doctest.html
[google-style-guide]: https://google.github.io/styleguide/pyguide.html
[mypy]: https://mypy.readthedocs.io/
[mypy-error-codes]: https://mypy.readthedocs.io/en/stable/error_code_list.html
[pre-commit]: https://pre-commit.com/
[pylint]: https://pylint.pycqa.org/
[ruff-formatter]: https://docs.astral.sh/ruff/formatter/
[ruff-linter]: https://docs.astral.sh/ruff/linter/
[ruff-rules]: https://docs.astral.sh/ruff/rules/
[sphinx]: https://www.sphinx-doc.org
[sphinx-autodoc]: https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html
[sphinx-napoleon]: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/index.html#
[sphinx-rest]: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ python3.10 -m venv .venv

# Activate the virtual environment and make sure that 'wheel' is installed
source .venv/bin/activate
pip install --upgrade wheel
pip install --upgrade wheel pip setuptools

# Install all the ICON4Py packages and its dependencies
# External dependencies would be checked out at './_external_src'
Expand Down
2 changes: 1 addition & 1 deletion ci/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ test_model_stencils:
# exclude slow test configurations
- if: $BACKEND == "roundtrip" && $GRID == "icon_grid"
when: never
- when: always
- when: on_success

test_tools:
extends: .test_template
Expand Down
3 changes: 2 additions & 1 deletion model/atmosphere/advection/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ classifiers = [
]
dependencies = [
"gt4py>=1.0.1",
"icon4py-common>=0.0.5"
"icon4py-common>=0.0.5",
"nanobind<2.0.0"
]
description = "ICON advection."
dynamic = ['version']
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ def _btraj_dreg_stencil_02(
lvn_pos = where(p_vn >= 0.0, True, False)
traj_length = sqrt(p_vn * p_vn + p_vt * p_vt) * p_dt
e2c_length = where(lvn_pos, edge_cell_length(E2EC[0]), edge_cell_length(E2EC[1]))
opt_famask_dsl = where(
traj_length > 1.25 * broadcast(e2c_length, (EdgeDim, KDim)), int32(1), int32(0)
)
opt_famask_dsl = where(traj_length > 1.25 * broadcast(e2c_length, (EdgeDim, KDim)), 1, 0)

return opt_famask_dsl

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,37 @@ def _btraj_dreg_stencil_03(
Field[[EdgeDim, KDim], float],
Field[[EdgeDim, KDim], float],
]:
# logical switch for MERGE operations: True for p_vn >= 0
lvn_pos = where(p_vn >= 0.0, True, False)

# get line and block indices of upwind cell
p_cell_idx = where(lvn_pos, cell_idx(E2EC[0]), cell_idx(E2EC[1]))
p_cell_rel_idx_dsl = where(lvn_pos, int32(0), int32(1))
p_cell_rel_idx_dsl = where(lvn_pos, 0, 1)
p_cell_blk = where(lvn_pos, cell_blk(E2EC[0]), cell_blk(E2EC[1]))

# departure points of the departure cell. Point 1 belongs to edge-vertex 1,
# point 2 belongs to edge_vertex 2.
#
# position of vertex 4 (vn > 0) / vertex 2(vn < 0) in normal direction
depart_pts_1_x = edge_verts_1_x - p_vn * p_dt
# position of vertex 4 (vn > 0) / vertex 2(vn < 0) in tangential direction
depart_pts_1_y = edge_verts_1_y - p_vt * p_dt
# position of vertex 3 in normal direction
depart_pts_2_x = edge_verts_2_x - p_vn * p_dt
# position of vertex 3 in tangential direction
depart_pts_2_y = edge_verts_2_y - p_vt * p_dt

# determine correct position on tangential plane
pos_on_tplane_e_x = where(lvn_pos, pos_on_tplane_e_1_x, pos_on_tplane_e_2_x)
pos_on_tplane_e_y = where(lvn_pos, pos_on_tplane_e_1_y, pos_on_tplane_e_2_y)

# Calculate position of departure region vertices in a translated
# coordinate system. The origin is located at the circumcenter
# of the upwind cell. The distance vectors point from the cell center
# to the vertices.

# Take care of correct counterclockwise numbering below

pos_dreg_vert_c_1_x = edge_verts_1_x - pos_on_tplane_e_x
pos_dreg_vert_c_1_y = edge_verts_1_y - pos_on_tplane_e_y
pos_dreg_vert_c_2_x = where(lvn_sys_pos, depart_pts_1_x, edge_verts_2_x) - pos_on_tplane_e_x
Expand All @@ -73,15 +90,24 @@ def _btraj_dreg_stencil_03(
pos_dreg_vert_c_4_x = where(lvn_sys_pos, edge_verts_2_x, depart_pts_1_x) - pos_on_tplane_e_x
pos_dreg_vert_c_4_y = where(lvn_sys_pos, edge_verts_2_y, depart_pts_1_y) - pos_on_tplane_e_y

# In a last step, these distance vectors are transformed into a rotated
# geographical coordinate system, which still has its origin at the circumcenter
# of the upwind cell. Now the coordinate axes point to local East and local
# North.

# Determine primal and dual normals of the cell lying in the direction of vn
pn_cell_1 = where(lvn_pos, primal_normal_cell_x(E2EC[0]), primal_normal_cell_x(E2EC[1]))
pn_cell_2 = where(lvn_pos, primal_normal_cell_y(E2EC[0]), primal_normal_cell_y(E2EC[1]))
dn_cell_1 = where(lvn_pos, dual_normal_cell_x(E2EC[0]), dual_normal_cell_x(E2EC[1]))
dn_cell_2 = where(lvn_pos, dual_normal_cell_y(E2EC[0]), dual_normal_cell_y(E2EC[1]))

# components in longitudinal direction
p_coords_dreg_v_1_lon_dsl = pos_dreg_vert_c_1_x * pn_cell_1 + pos_dreg_vert_c_1_y * dn_cell_1
p_coords_dreg_v_2_lon_dsl = pos_dreg_vert_c_2_x * pn_cell_1 + pos_dreg_vert_c_2_y * dn_cell_1
p_coords_dreg_v_3_lon_dsl = pos_dreg_vert_c_3_x * pn_cell_1 + pos_dreg_vert_c_3_y * dn_cell_1
p_coords_dreg_v_4_lon_dsl = pos_dreg_vert_c_4_x * pn_cell_1 + pos_dreg_vert_c_4_y * dn_cell_1

# components in latitudinal direction
p_coords_dreg_v_1_lat_dsl = pos_dreg_vert_c_1_x * pn_cell_2 + pos_dreg_vert_c_1_y * dn_cell_2
p_coords_dreg_v_2_lat_dsl = pos_dreg_vert_c_2_x * pn_cell_2 + pos_dreg_vert_c_2_y * dn_cell_2
p_coords_dreg_v_3_lat_dsl = pos_dreg_vert_c_3_x * pn_cell_2 + pos_dreg_vert_c_3_y * dn_cell_2
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,15 @@ def line_intersect(
line2_p2_lon: Field[[EdgeDim, KDim], float],
line2_p2_lat: Field[[EdgeDim, KDim], float],
) -> tuple[Field[[EdgeDim, KDim], float], Field[[EdgeDim, KDim], float]]:
m1 = (line1_p2_lat - line1_p1_lat) / (line1_p2_lon - line1_p1_lon)
m2 = (line2_p2_lat - line2_p1_lat) / (line2_p2_lon - line2_p1_lon)
# avoid division with zero
d1 = line1_p2_lon - line1_p1_lon
d1 = where(d1 != 0.0, d1, line1_p2_lon)

d2 = line2_p2_lon - line2_p1_lon
d2 = where(d2 != 0.0, d2, line2_p2_lon)

m1 = (line1_p2_lat - line1_p1_lat) / d1
m2 = (line2_p2_lat - line2_p1_lat) / d2

intersect_1 = (line2_p1_lat - line1_p1_lat + m1 * line1_p1_lon - m2 * line2_p1_lon) / (m1 - m2)
intersect_2 = line1_p1_lat + m1 * (intersect_1 - line1_p1_lon)
Expand Down Expand Up @@ -228,7 +235,7 @@ def _divide_flux_area_list_stencil_01(
lvn_sys_pos = where(
(p_vn * broadcast(tangent_orientation_dsl, (EdgeDim, KDim))) >= 0.0, True, False
)
famask_bool = where(famask_int == int32(1), True, False)
famask_bool = where(famask_int == 1, True, False)
# ------------------------------------------------- Case 1
mask_case1 = lintersect_line1 & lintersect_line2 & famask_bool
ps1_x, ps1_y = line_intersect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def _divide_flux_area_list_stencil_02(
Field[[EdgeDim, KDim], int32],
Field[[EdgeDim, KDim], int32],
]:
famask_bool = where(famask_int == int32(1), True, False)
famask_bool = where(famask_int == 1, True, False)
lvn_pos = where(p_vn >= 0.0, True, False)
# Translation of patch 1 and patch 2 in system relative to respective cell
bf_cc_patch1_lon = where(
Expand Down Expand Up @@ -133,22 +133,22 @@ def _divide_flux_area_list_stencil_02(
patch1_cell_idx_vmask = where(
famask_bool,
where(lvn_pos, butterfly_idx_patch1_vnpos_3d, butterfly_idx_patch1_vnneg_3d),
int32(0),
0,
)
patch2_cell_idx_vmask = where(
famask_bool,
where(lvn_pos, butterfly_idx_patch2_vnpos_3d, butterfly_idx_patch2_vnneg_3d),
int32(0),
0,
)
patch1_cell_blk_vmask = where(
famask_bool,
where(lvn_pos, butterfly_blk_patch1_vnpos_3d, butterfly_blk_patch1_vnneg_3d),
int32(0),
0,
)
patch2_cell_blk_vmask = where(
famask_bool,
where(lvn_pos, butterfly_blk_patch2_vnpos_3d, butterfly_blk_patch2_vnneg_3d),
int32(0),
0,
)

return (
Expand Down
Loading

0 comments on commit 70223e1

Please sign in to comment.