Zco: More xarray, tests, and improve/expose docstring #31
Conversation
Codecov Report
@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 93.12% 94.05% +0.92%
==========================================
Files 4 4
Lines 160 185 +25
==========================================
+ Hits 149 174 +25
Misses 11 11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
malmans2
left a comment
malmans2
left a comment
There was a problem hiding this comment.
- No more loops over
z - Documentation in good shape
- A couple of additional tests
- We're now fully checking types (before
xarrayobjects were replaced byAny). All functions arguments and returns have a type annotation. From now on mypy should be more helpful than annoying. If you add type annotation as soon as you define new functions, it should be easy to know if we have to make changes upstream
| ldbletanh: bool, optional | ||
| Logical flag to switch ON/OFF the double tanh stretching function. | ||
| This flag is only needed for compatibility with NEMO DOMAINcfg tools. | ||
| Just set ``ppa2``, ``ppkth2``, and ``ppacr2`` to switch ON | ||
| the double tanh stretching function. |
There was a problem hiding this comment.
I think this flag is redundant. I suggest we keep it just for backward compatibility with NEMO tools.
| ppa1 = aa / (bb - cc * (dd - ee)) if is_nemo_none(ppa1) else ppa1 | ||
| ppa0 = self._ppdzmin - ppa1 * bb if is_nemo_none(ppa0) else ppa0 | ||
| ppsur = -(ppa0 + ppa1 * self._ppacr * ee) if is_nemo_none(ppsur) else ppsur | ||
| # Substitute only if is None or 999999 | ||
| ppa1_out = (aa / (bb - cc * (dd - ee))) if _is_nemo_none(ppa1) else ppa1 | ||
| ppa0_out = (self._ppdzmin - ppa1_out * bb) if _is_nemo_none(ppa0) else ppa0 | ||
| ppsur_out = ( | ||
| -(ppa0_out + ppa1_out * self._ppacr * ee) if _is_nemo_none(ppsur) else ppsur | ||
| ) |
There was a problem hiding this comment.
Just helping out mypy...
pydomcfg/domzgr/zco.py
Outdated
| # Errors | ||
| if ldbletanh is True and any(pp_are_none): | ||
| raise ValueError(f"{prefix_msg} MUST be all float when ldbletanh is True") | ||
| if ldbletanh is None and (any(pp_are_none) and not all(pp_are_none)): | ||
| raise ValueError(f"{prefix_msg} MUST be all None or float") | ||
|
|
||
| # Warning | ||
| if ldbletanh is False and not all(pp_are_none): | ||
| warnings.warn(f"{prefix_msg} are ignored when ldbletanh is False") |
There was a problem hiding this comment.
Does this look right? Before we were doing a more strict check (pp>0 rather than not null), but I think it makes more sense to default None and the user can fully play with the underlying equation. If you think we should restore pp>0 when ldbletanh is True, then maybe we should introduce more checks on all parameters (e.g., ppdzmin >= 0, ...)
| # ------------------------------------------------------------------------- | ||
| def _init_ds(self): | ||
| """ | ||
| Initialise the xarray dataset with empty | ||
| ``z3{T,W}`` and ``e3{T,W}`` | ||
| Returns | ||
| ------- | ||
| ds: Dataset | ||
| A copy of the dataset used to initialise the class with new | ||
| coordinates ``z3{T,W}`` and ``e3{T,W}`` empty dataarrays | ||
| """ | ||
| ds = self._bathy.copy() | ||
|
|
||
| var = ["z3", "e3"] | ||
| grd = ["T", "W"] | ||
|
|
||
| # Initialise a dataset with empty z3 and e3 dataarrays | ||
| da = xr.full_like(ds["Bathymetry"], None).expand_dims(z=range(self._jpk)) | ||
| for v, g in product(var, grd): | ||
| ds[v + g] = da.copy() | ||
| ds = ds.set_coords(v + g) | ||
| return ds | ||
| # Z dimension | ||
| self._z = DataArray(range(jpk), dims="z") |
There was a problem hiding this comment.
We don't need to initialize the dataset anymore. If you want to start Sco with a similar approach (i.e., start with loops, then remove them) you'd have to re-introduce this.
malmans2
added
documentation
malmans2
changed the title
|
@malmans2 - are you happy with this updated PR ... I noticed the flurry of activity so waited to review - any more additions? I'll go over the updates after the NEMO meeting this morning. |
|
Yes, it's ready for review. |
xarrayauto-broadcasting #19ZcoandZgrclasses to public API #25pre-commit run --all-filesapi.rst