Developing sco class #33

oceandie · 2021-06-15T20:27:38Z

Closes #xxxx
Tests added
Passes pre-commit run --all-files
Changes are documented in whats-new.rst
New functions/methods are listed in api.rst
Project, label, and assignee tabs are populated

codecov · 2021-06-15T20:29:08Z

Codecov Report

Merging #33 (1d80a84) into main (9002bb0) will decrease coverage by 13.18%.
The diff coverage is 14.63%.

@@             Coverage Diff             @@
##             main      #33       +/-   ##
===========================================
- Coverage   94.85%   81.67%   -13.19%     
===========================================
  Files           5        5               
  Lines         214      251       +37     
===========================================
+ Hits          203      205        +2     
- Misses         11       46       +35

Flag	Coverage Δ
unittests	`81.67% <14.63%> (-13.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pydomcfg/utils.py	`52.00% <5.40%> (-45.37%)`	⬇️
pydomcfg/domzgr/zco.py	`92.38% <100.00%> (ø)`
pydomcfg/domzgr/zgr.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9002bb0...1d80a84. Read the comment docs.

oceandie · 2021-06-15T20:53:06Z

pyDOMCFG/pydomcfg/domzgr/sco.py

Line 114 in 80b4b58

if self._stretch:

Worthy to implement the same strategy for zco? ... will allow to get rid of the quite annoying ldbletanh boolean flag ...

malmans2

I introduced a few changes in Zgr with #31.
I think it makes sense to review and merge #31, so we build Sco starting from the most recent version.

Also, mypy is fully integrated in #31, while here you are not really checking xarray objects as xarray is not an additional dependency in the config file.

The only major change that #31 introduces is that _init_ds is gone, as we don't have to initialize e3 and z3 variables anymore. But If you want to start translating from fortran using it, I would introduce _init_ds in the child class. I.e., we will do something like for Zco (1. translate from fortran, 2. refactor to make more use of xarray) but we don't have to change/refactor anything in the base class.

malmans2 · 2021-06-16T08:13:26Z

pydomcfg/domzgr/sco.py

+        # set stretching parameters after checking their consistency
+        if self._stretch:
+            self._set_stretch_par(psurf, pbott, alpha, efold, pbot2)


When we append variables to self in separate methods, they're kind of hidden. I would do something like in Zco, so self._set_stretch_par returns psurf, pbott, alpha, efold, pbot2.

Actually, what you are doing so far (basically self._psurf = psurf or None, ...) is easy enough that I think you can set these variables directly in __call__, and rename the function to something like check_stetch_par.

malmans2 · 2021-06-16T08:15:32Z

pydomcfg/domzgr/sco.py

+        if self._stretch:
+            self._set_stretch_par(psurf, pbott, alpha, efold, pbot2)
+
+        ds = self._init_ds()


_init_ds is gone in #31 as we don't need to initialize e3 and z3 variables anymore.

malmans2 · 2021-06-16T08:16:21Z

pydomcfg/domzgr/sco.py

+        ds_env = self._compute_env(ds)
+
+        # compute sigma-coordinates for z3 computation
+        kindx = ds_env["z"]


When we merge #31 you can do this:

kindx = self._z

malmans2 · 2021-06-16T08:17:08Z

pydomcfg/domzgr/sco.py

+
+        # compute sigma-coordinates for z3 computation
+        kindx = ds_env["z"]
+        sigma = (self._compute_sigma(kk) for kk in kindx)


Should this be:

Suggested change

sigma = (self._compute_sigma(kk) for kk in kindx)

sigma = self._compute_sigma(kindx)

malmans2 · 2021-06-16T08:26:51Z

pydomcfg/domzgr/sco.py

+            msg = (
+                srf
+                + " and "
+                + bot
+                + "MUST be set when using "
+                + self._stretch
+                + " stretching."
+            )
+            raise ValueError(msg)


Suggested change

msg = (

srf

+ " and "

+ bot

+ "MUST be set when using "

+ self._stretch

+ " stretching."

)

raise ValueError(msg)

raise ValueError(

f"{srf} and {bot} MUST be set when using {self._stretch} stretching."

)

malmans2 · 2021-06-16T08:28:09Z

pydomcfg/domzgr/sco.py

+        self._psurf = psurf if psurf else 0.0
+        self._pbott = pbott if pbott else 0.0
+        self._alpha = alpha if alpha else 0.0
+        self._efold = efold if efold else 0.0
+        self._pbot2 = pbot2 if pbot2 else 0.0


Suggested change

self._psurf = psurf if psurf else 0.0

self._pbott = pbott if pbott else 0.0

self._alpha = alpha if alpha else 0.0

self._efold = efold if efold else 0.0

self._pbot2 = pbot2 if pbot2 else 0.0

self._psurf = psurf or 0

self._pbott = pbott or 0

self._alpha = alpha or 0

self._efold = efold or 0

self._pbot2 = pbot2 or 0

Also, I think you can do this directly in __call__

malmans2 · 2021-06-16T08:30:03Z

pydomcfg/utils.py

@@ -26,6 +27,51 @@ def is_nemo_none(var: Optional[float] = None) -> bool:
    return var in [None, 999999.0]


+# -----------------------------------------------------------------------------
+def calc_rmax(depth: DataArray) -> DataArray:


Same used by the Bathymetry class, right?
Then we could delete it in tests/bathymetry.py and import from here.

malmans2 · 2021-06-16T08:58:07Z

pyDOMCFG/pydomcfg/domzgr/sco.py

Line 114 in 80b4b58

if self._stretch:

Worthy to implement the same strategy for zco? ... will allow to get rid of the quite annoying ldbletanh boolean flag ...

Right, although I think the best we can do with ldbletanh is to work around it because it's defined in the namelist.
Is this what you mean?

pyDOMCFG/pydomcfg/domzgr/zco.py

Line 288 in 0bc4f0b

ldbletanh_out = ldbletanh if (ldbletanh is not None) else not any(pp_are_none)

I.e., default is None, set to True/False if the user defines it, otherwise False if the tanh parameters are not set (and viceversa).

malmans2 · 2021-06-16T11:07:57Z

pydomcfg/domzgr/sco.py

+            if self._stretch == "sh94":
+                srf = "rn_theta"
+                bot = "rn_bb"
+            elif self._stretch == "md96":
+                srf = "rn_theta"
+                bot = "rn_thetb"
+            elif self._stretch == "sf12":
+                srf = "rn_zs"
+                bot = "rn_zb_a"


Missing a final else with a slightly different error (i.e., typo or the stretch is not supported)

jdha

calc_rmax is now in utils.py which I guess makes _calc_rmax in tests/bathymetry.py redundant. Unless told otherwise I'll raise an issue to tidy after this PR.

oceandie · 2021-06-16T11:23:00Z

@malmans2 Yes I know, all your comments are correct but as I specified in the committ this was just to start the work, so just a quick structure of the module, surely not ready for review ;)
Considering your recent modifications (and the next ones after @jdha review) to the main branch, I will delete this PR and associated branch and start from a clean one directly from the more recent main.

jdha · 2021-06-16T11:24:05Z

OK - I should have read the whole thread first. @malmans2 has already commented on calc_rmax, perhaps we just tidy in this PR (that would be me being lazy - @malmans2: would you prefer things like this to be dealt with as separate issues?)

oceandie · 2021-06-16T11:25:55Z

pyDOMCFG/pydomcfg/domzgr/sco.py

Line 114 in 80b4b58

if self._stretch:

Worthy to implement the same strategy for zco? ... will allow to get rid of the quite annoying ldbletanh boolean flag ...

Right, although I think the best we can do with ldbletanh is to work around it because it's defined in the namelist.
Is this what you mean?

pyDOMCFG/pydomcfg/domzgr/zco.py

Line 288 in 0bc4f0b

ldbletanh_out = ldbletanh if (ldbletanh is not None) else not any(pp_are_none)

I.e., default is None, set to True/False if the user defines it, otherwise False if the tanh parameters are not set (and viceversa).

My idea is to introduce an intermediate step (function) where we translate (map) from NEMO-like namelist parameters to pyDOMCFG-like parameters.

malmans2 · 2021-06-16T11:33:24Z

@jdha I think it would make more sense to do it here

@oceandie no need to open a new PR, we'll just rebase once we merge #31
Also, I suggest to convert this PR into a draft (top right, under reviewer), and then change the status when you are ready for a full review. That's how I view it:

I don't want any feedback: Fork on you private account, don't open a PR
Draft PR: I'm OK with feedback if you have time to glance through, but it's really work in progress so don't spend too much time on it.
Remove the draft status: I want to merge this, ready for a full review and I need someone's approval

malmans2 · 2021-06-16T11:37:29Z

My idea is to introduce an intermediate step (function) where we translate (map) from NEMO-like namelist parameters to pyDOMCFG-like parameters.

I like it, it would make everything much easier! I suggest we open an issue about this and we address it in a separate PR.

jdha · 2021-06-16T11:49:22Z

@jdha I think it would make more sense to do it here

@oceandie no need to open a new PR, we'll just rebase once we merge #31
Also, I suggest to convert this PR into a draft (top right, under reviewer), and then change the status when you are ready for a full review. That's how I view it:

I don't want any feedback: Fork on you private account, don't open a PR

Draft PR: I'm OK with feedback if you have time to glance through, but it's really work in progress so don't spend too much time on it.

Remove the draft status: I want to merge this, ready for a full review and I need someone's approval

(alternative) create a local branch and when you're ready set up an upstream branch, push and open a PR

oceandie · 2021-06-16T12:04:10Z

@jdha I think it would make more sense to do it here
@oceandie no need to open a new PR, we'll just rebase once we merge #31
Also, I suggest to convert this PR into a draft (top right, under reviewer), and then change the status when you are ready for a full review. That's how I view it:

I don't want any feedback: Fork on you private account, don't open a PR

Draft PR: I'm OK with feedback if you have time to glance through, but it's really work in progress so don't spend too much time on it.

Remove the draft status: I want to merge this, ready for a full review and I need someone's approval
1. (alternative) create a local branch and when you're ready set up an upstream branch, push and open a PR

Perfect, thanks a lot both for the clarifications - I thought opening a PR was kind of "mandatory" everytime you open a branch ... good to know ;) I think for this one I will do as suggested by @malmans2, I'll put draft status

malmans2 · 2021-06-16T15:45:50Z

@oceandie you'll need to merge main into this. The only conflict with main is basically just the # ------ comment you introduced (I personally don't love those kind of separators because of these conflicts + python indentation already works pretty good, but it's really a personal preference).

Anyways, if you haven't done it before, here is what I would do from terminal

git clone https://github.com/pyNEMO/pyDOMCFG.git
cd pyDOMCFG
git fetch origin
git checkout -b sco_dev origin/sco_dev
git merge main

Fix conflicts:
Open pydomcfg/utils.py and remove everything between <<<<<<< HEAD and >>>>>>> main.

Git add, commit and push sco_dev as usual.

~~(I think you can do it from GH as well, but I don't see the button. Maybe you do as it's your draft PR?)~~
Edit: Actually the Resolve conflicts button showed up for me as well...

malmans2 · 2021-06-19T18:07:51Z

Don't worry about the CI/upstream-dev action failing. We'll add typing_extensions to the environment file if it doesn't get fixed upstream in a few days.

malmans2 · 2021-06-21T08:33:03Z

pydomcfg/utils.py

+    # Do we actually need this? mypy complains since
+    # DataArray.reset_indexe() returns Optional["DataArray"]
+    #
+    # depth = depth.reset_index(list(depth.dims))


Interesting, not sure why they assigned that type annotation to reset_index. One quick solution is DataArray(depth.reset_index(list(depth.dims))).

_calc_rmax doesn't really need this, the problem is that if we don't use it a DataArray with different indexes is returned. This means that we'll get conflicts when we try to merge in the main dataset.

I was just lazy though... it's quite easy in this case to properly use the indexes. I'll open a separate PR for that as you haven't removed calc_rmax from Bathymetry yet.

Ok, I will implement the quick solution DataArray(depth.reset_index(list(depth.dims))) for now i my branch since it seems we need it also in _calc_rmax: if I remove it, I get the following error:
ValueError: cannot reindex or align along dimension 'y' because the index has duplicate values

BTW, forza azzurri !!!!

oceandie · 2021-06-21T11:27:40Z

pydomcfg/utils.py

@@ -59,6 +56,8 @@ def _calc_rmax(depth: DataArray) -> float:
        depth_diff = depth.diff(dim)
        depth_rolling_sum = depth.rolling({dim: 2}).sum().dropna(dim)
        rmax = depth_diff / depth_rolling_sum
+        # dealing with nans at land points


@jdha and @malmans2 I think we need something like this in #40 for managing nans (0/0) in the case of land points

Update to PR 40

pydomcfg/domzgr/sco.py

Co-authored-by: Mattia Almansi <[email protected]>

oceandie · 2021-06-29T17:43:05Z

not sure why mypy is failing ... any idea?

pydomcfg/domzgr/sco.py

* refactor to avoid loops * apply Diego's suggestions * remove useless return after error

malmans2 · 2021-07-05T13:33:58Z

Looks like there aren't conflicts right now, so I'd merge main into sco_dev whenever you get a chance.
Once you merge, can you please try adding the decorator to Sco __call__? See:

pyDOMCFG/pydomcfg/domzgr/zco.py

Lines 21 to 22 in 6888e41

    
           @_check_parameters 
        
           def __call__(

oceandie added 3 commits June 15, 2021 21:19

including calc_rmax in utils to be used with sco class

4fb1bdd

including calc_rmax in utils to be used with sco class

6d31636

including calc_rmax in utils to be used with sco class

5d0926c

start work on sco

80b4b58

malmans2 mentioned this pull request Jun 16, 2021

Zco: More xarray, tests, and improve/expose docstring #31

Merged

6 tasks

malmans2 reviewed Jun 16, 2021

View reviewed changes

jdha reviewed Jun 16, 2021

View reviewed changes

oceandie marked this pull request as draft June 16, 2021 12:05

oceandie added 2 commits June 19, 2021 17:31

merging main to sco_dev

9bf2842

merging main to sco_dev and adding _calc_rmax to utils

c8a28f5

malmans2 reviewed Jun 21, 2021

View reviewed changes

add computation of envelope bathymetry

be52a5c

oceandie commented Jun 21, 2021

View reviewed changes

malmans2 mentioned this pull request Jun 21, 2021

Do not drop indexes when computing rmax #40

Merged

4 tasks

oceandie added 3 commits June 25, 2021 21:23

amending doc

966bf23

little test just for me

4567fde

little test just for me

8b90ea9

oceandie added 4 commits June 27, 2021 16:34

Merge branch 'main' of https://github.com/pyNEMO/pyDOMCFG into sco_dev

3f70195

Update to PR 40

cleaning

1dd355e

add envelope computation

eac503b

cleaning syntax

9589679

malmans2 reviewed Jun 29, 2021

View reviewed changes

pydomcfg/domzgr/sco.py Outdated Show resolved Hide resolved

oceandie and others added 5 commits June 29, 2021 14:57

Fix little inconsistencies at the boundaries

3119aad

Co-authored-by: Mattia Almansi <[email protected]>

cleaning

ef02eec

updating name of stretched var for consistency

02842f7

updating name of stretched var for consistency

bb206e2

add sh94 and md96 stretching and computation of gdep and e3

6750533

malmans2 reviewed Jun 30, 2021

View reviewed changes

pydomcfg/domzgr/sco.py Outdated Show resolved Hide resolved

malmans2 mentioned this pull request Jun 30, 2021

Refactor MB06 #51

Merged

malmans2 and others added 2 commits June 30, 2021 11:41

Refactor MB06 (#51)

8e08398

* refactor to avoid loops * apply Diego's suggestions * remove useless return after error

implementing Mattia's mypy workaround

1d80a84

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Developing sco class #33

Developing sco class #33

oceandie commented Jun 15, 2021

codecov bot commented Jun 15, 2021 •

edited

Loading

oceandie commented Jun 15, 2021

malmans2 left a comment •

edited

Loading

malmans2 Jun 16, 2021 •

edited

Loading

malmans2 Jun 16, 2021

malmans2 Jun 16, 2021

malmans2 Jun 16, 2021

malmans2 Jun 16, 2021

malmans2 Jun 16, 2021

malmans2 Jun 16, 2021

malmans2 commented Jun 16, 2021 •

edited

Loading

malmans2 Jun 16, 2021

jdha left a comment

oceandie commented Jun 16, 2021 •

edited

Loading

jdha commented Jun 16, 2021 •

edited

Loading

oceandie commented Jun 16, 2021

malmans2 commented Jun 16, 2021

malmans2 commented Jun 16, 2021

jdha commented Jun 16, 2021

oceandie commented Jun 16, 2021

malmans2 commented Jun 16, 2021 •

edited

Loading

malmans2 commented Jun 19, 2021 •

edited

Loading

malmans2 Jun 21, 2021

oceandie Jun 21, 2021

oceandie Jun 21, 2021

oceandie Jun 21, 2021 •

edited

Loading

oceandie commented Jun 29, 2021

malmans2 commented Jul 5, 2021

	sigma = (self._compute_sigma(kk) for kk in kindx)
	sigma = self._compute_sigma(kindx)

Developing sco class #33

Are you sure you want to change the base?

Developing sco class #33

Conversation

oceandie commented Jun 15, 2021

codecov bot commented Jun 15, 2021 • edited Loading

Codecov Report

oceandie commented Jun 15, 2021

malmans2 left a comment • edited Loading

Choose a reason for hiding this comment

malmans2 Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malmans2 commented Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

jdha left a comment

Choose a reason for hiding this comment

oceandie commented Jun 16, 2021 • edited Loading

jdha commented Jun 16, 2021 • edited Loading

oceandie commented Jun 16, 2021

malmans2 commented Jun 16, 2021

malmans2 commented Jun 16, 2021

jdha commented Jun 16, 2021

oceandie commented Jun 16, 2021

malmans2 commented Jun 16, 2021 • edited Loading

malmans2 commented Jun 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oceandie Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

oceandie commented Jun 29, 2021

malmans2 commented Jul 5, 2021

codecov bot commented Jun 15, 2021 •

edited

Loading

malmans2 left a comment •

edited

Loading

malmans2 Jun 16, 2021 •

edited

Loading

malmans2 commented Jun 16, 2021 •

edited

Loading

oceandie commented Jun 16, 2021 •

edited

Loading

jdha commented Jun 16, 2021 •

edited

Loading

malmans2 commented Jun 16, 2021 •

edited

Loading

malmans2 commented Jun 19, 2021 •

edited

Loading

oceandie Jun 21, 2021 •

edited

Loading