add zgr and zco classes #15

oceandie · 2021-06-07T08:43:15Z

No description provided.

codecov · 2021-06-07T08:44:53Z

Codecov Report

Merging #15 (e94dad4) into main (687d67c) will increase coverage by 0.81%.
The diff coverage is 93.38%.

@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   92.30%   93.12%   +0.81%     
==========================================
  Files           2        4       +2     
  Lines          39      160     +121     
==========================================
+ Hits           36      149     +113     
- Misses          3       11       +8

Flag	Coverage Δ
unittests	`93.12% <93.38%> (+0.81%)`	⬆️

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

Impacted Files	Coverage Δ
pydomcfg/domzgr/zco.py	`90.90% <90.90%> (ø)`
pydomcfg/domzgr/zgr.py	`100.00% <100.00%> (ø)`
pydomcfg/utils.py	`97.05% <100.00%> (+0.18%)`	⬆️

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 687d67c...e94dad4. Read the comment docs.

malmans2

I added comments that should make darglint happy.

pydomcfg/domzgr/zco.py

pydomcfg/domzgr/zgr.py

oceandie · 2021-06-07T10:15:34Z

Next step I am going to write the test. Thanks @malmans2 for the help with the bloody darglint ;) :)

oceandie · 2021-06-07T10:20:19Z

Any idea why the failing of CI / upstream-dev (pull_request)?

malmans2

I've only glanced through it, but here are a few xarray tips.

pydomcfg/domzgr/zgr.py

malmans2 · 2021-06-07T10:28:49Z

Any idea why the failing of CI / upstream-dev (pull_request)?

Must have been a connection hiccup. I re-triggered it and looks OK.

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

pydomcfg/domzgr/zgr.py

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

pydomcfg/domzgr/zco.py

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

pydomcfg/domzgr/zco.py

oceandie · 2021-06-08T09:31:40Z

Ok so let me just write down my next steps:

Implement analytical e3T/W (to keep compatibility with 3.6 and for testing)
Implement @malmans2 add zgr and zco classes #15 (comment) suggestion
Implement some of @malmans2 73008a4 suggestions for sigma, compute_z3, compute_e3
Write the testing

Any other things we want to add?

malmans2 · 2021-06-08T09:44:14Z

Ok so let me just write down my next steps:

Implement analytical e3T/W (to keep compatibility with 3.6 and for testing)

Implement @malmans2 #15 (comment) suggestion

Implement some of @malmans2 73008a4 suggestions for sigma, compute_z3, compute_e3

Write the testing

Any other things we want to add?

Looks good. As you are not going to add that much at this point, I think 3 can wait after 4 so we can safely refactor if we need to. Just one comment about testing, I think the best way is to only test public methods (aka, easier to maintain and if we don't hit all private methods testing public methods, something is not right... codecov will tell us). Zco is already in good shape, I think you can make pretty much all methods in Zgr private as well...

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

malmans2

There a few times where I think for loops of the kind for var in (varT, varK) clarify when the same thing is done and will prevent typos and bugs...
I've used GH inline comments without running the code so please double check if you are going to implement it.

Also, does it make sense to isolate the entire for loop in __call__ that generates ds["z3{T,W}"] in a separate method? That way __call__ is very clean and just glancing through it shows all the steps that are done.

pydomcfg/domzgr/zco.py

pydomcfg/domzgr/zgr.py

malmans2 · 2021-06-09T09:16:51Z

There a few times where I think for loops of the kind for var in (varT, varK) clarify when the same thing is done and will prevent typos and bugs...
I've used GH inline comments without running the code so please double check if you are going to implement it.

I also just realized that all the inline for loops in __call__ can probably be condensed in one outer for loop:

for varname, kx, sig, sig_p1 in zip(["z3T", "z3W"], [kindx, kindx + 1.], sigmas, sigmas_p1):

so pretty much the same as this: #15 (comment)

malmans2 · 2021-06-09T09:33:40Z

When we have the tests, I think this will be the way to go:
Instead of having T and W variables stored directly in the class, we can define two separate datasets since the very beginning with identical variable names: e.g., self._ds_t, self._ds_w).
A the very end, we append to the variable names "t" or "w" and we merge the two datasets (removing variables that we don't want to output, such as sig).

malmans2

Final minor comments...

pydomcfg/domzgr/zco.py

pydomcfg/domzgr/zgr.py

pydomcfg/domzgr/zco.py

pydomcfg/tests/test_zco.py

malmans2 · 2021-06-10T13:30:23Z

pydomcfg/tests/test_zco.py

+    jpk = 31
+
+    # Bathymetry dataset
+    ds_bathy = Bathymetry(1000.0, 1200.0, 100, 200).flat(5000.0)


What if we test:

ds_bathy = Bathymetry(1000.0, 1200.0, 1, 1).flat(5000.0)

and possibly we have a separate test to make sure that all z/e3 are identical for 2D flat domains?

Sorry but I don't understand what you mean ...

https://github.com/pyNEMO/pyDOMCFG/pull/15/files#r649524600

pydomcfg/domzgr/zco.py

oceandie · 2021-06-10T15:16:11Z

This is the code that fail mypy

pydomcfg/tests/test_zco.py

pydomcfg/domzgr/zco.py

malmans2 · 2021-06-11T12:44:31Z

pydomcfg/domzgr/zco.py

+        if not self._is_uniform:
+            self._ppsur, self._ppa0, self._ppa1 = self._compute_pp(ppsur, ppa0, ppa1)


I think we are missing an error here, i.e. the case where is uniform and the user provides any of [ppsur, ppa0, ppa1]

If the user wants a uniform grid (acr or kth == 0) then we don't need ppsur, ppa0 and ppa1 at all - they are not used. So even if the user provide ppsur, ppa0 and ppa1, the condition of uniform grid prevent to set them. I think we are safe like this.

OK! Still the user is probably misunderstanding something... Let's add a warning?

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

add zgr and zco classes - darglint failing

210cbd0

oceandie added 2 commits June 7, 2021 09:57

cleaning code

5d05860

fixing docstring

9830bf3

malmans2 reviewed Jun 7, 2021

View reviewed changes

oceandie added 3 commits June 7, 2021 10:56

fixing docstring

e65cbcb

fixing docstring

0ee0788

fixing docstring

20027e6

malmans2 reviewed Jun 7, 2021

View reviewed changes

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

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

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

oceandie and others added 3 commits June 7, 2021 11:32

Update pydomcfg/domzgr/zgr.py

8cb2f05

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

bugfix

e15088d

implementing malmans2 suggestions

13ce6d3

malmans2 reviewed Jun 7, 2021

View reviewed changes

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

malmans2 reviewed Jun 7, 2021

View reviewed changes

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

Update pydomcfg/domzgr/zgr.py

aa91245

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

malmans2 reviewed Jun 7, 2021

View reviewed changes

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

oceandie and others added 2 commits June 7, 2021 14:32

Update pydomcfg/domzgr/zgr.py

4f7cb39

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

implement malmans2 suggestions

ce734d8

oceandie changed the title ~~add zgr and zco classes - darglint failing~~ add zgr and zco classes Jun 7, 2021

oceandie added 2 commits June 7, 2021 21:18

cleaning and simplifying

acf253a

bugfix

d0194b4

malmans2 reviewed Jun 8, 2021

View reviewed changes

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

jdha mentioned this pull request Jun 8, 2021

remove for loops #16

Closed

oceandie added 2 commits June 8, 2021 11:11

implement malmans2 suggestion

8ea2e8b

implement analytical e3 and convert zgr func to n-dim arrays

ffd97bf

oceandie and others added 3 commits June 9, 2021 08:05

Update pydomcfg/domzgr/zco.py

1a32f08

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

removing grd argument from sigma func

45e6e95

moving inp param error to the very top

3dbd484

malmans2 reviewed Jun 9, 2021

View reviewed changes

oceandie added 2 commits June 9, 2021 16:04

simplifying kindx

97abf22

introducing looping over T and W grids

2ab3f24

malmans2 reviewed Jun 10, 2021

View reviewed changes

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

adding test for zco and bugfix

23160e3

malmans2 reviewed Jun 10, 2021

View reviewed changes

pydomcfg/tests/test_zco.py Outdated Show resolved Hide resolved

malmans2 reviewed Jun 10, 2021

View reviewed changes

pydomcfg/tests/test_zco.py Outdated Show resolved Hide resolved

malmans2 reviewed Jun 10, 2021

View reviewed changes

pydomcfg/tests/test_zco.py Outdated Show resolved Hide resolved

malmans2 reviewed Jun 10, 2021

View reviewed changes

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

pythonic way of managing 999999.0

b9bf03b

malmans2 and others added 4 commits June 10, 2021 20:42

pass arguments in compute_pp (#18)

c0b44d9

simplifying sigma computation

71a94c8

splitting test_zco()

690ecda

cleaning code

8766220

malmans2 reviewed Jun 10, 2021

View reviewed changes

pydomcfg/tests/test_zco.py Outdated Show resolved Hide resolved

malmans2 reviewed Jun 11, 2021

View reviewed changes

Update pydomcfg/tests/test_zco.py to 1D

5a4ab38

Co-authored-by: Mattia Almansi <mattia.almansi@noc.ac.uk>

malmans2 approved these changes Jun 11, 2021

View reviewed changes

update test_zco_uniform to 1D

e94dad4

oceandie linked an issue Jun 11, 2021 that may be closed by this pull request

Develop zgr and zco classes #21

Closed

oceandie merged commit 34ca3ca into main Jun 11, 2021

oceandie deleted the zco_dev branch June 12, 2021 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add zgr and zco classes #15

add zgr and zco classes #15

oceandie commented Jun 7, 2021

codecov bot commented Jun 7, 2021 •

edited

malmans2 left a comment

oceandie commented Jun 7, 2021

oceandie commented Jun 7, 2021

malmans2 left a comment

malmans2 commented Jun 7, 2021

oceandie commented Jun 8, 2021

malmans2 commented Jun 8, 2021

malmans2 left a comment

malmans2 commented Jun 9, 2021 •

edited

malmans2 commented Jun 9, 2021

malmans2 left a comment

malmans2 Jun 10, 2021 •

edited

oceandie Jun 10, 2021

malmans2 Jun 10, 2021

oceandie commented Jun 10, 2021

malmans2 Jun 11, 2021

oceandie Jun 11, 2021 •

edited

malmans2 Jun 11, 2021

malmans2 Jun 11, 2021

oceandie Jun 11, 2021

		if not self._is_uniform:
		self._ppsur, self._ppa0, self._ppa1 = self._compute_pp(ppsur, ppa0, ppa1)

add zgr and zco classes #15

add zgr and zco classes #15

Conversation

oceandie commented Jun 7, 2021

codecov bot commented Jun 7, 2021 • edited

Codecov Report

malmans2 left a comment

Choose a reason for hiding this comment

oceandie commented Jun 7, 2021

oceandie commented Jun 7, 2021

malmans2 left a comment

Choose a reason for hiding this comment

malmans2 commented Jun 7, 2021

oceandie commented Jun 8, 2021

malmans2 commented Jun 8, 2021

malmans2 left a comment

Choose a reason for hiding this comment

malmans2 commented Jun 9, 2021 • edited

malmans2 commented Jun 9, 2021

malmans2 left a comment

Choose a reason for hiding this comment

malmans2 Jun 10, 2021 • edited

Choose a reason for hiding this comment

oceandie Jun 10, 2021

Choose a reason for hiding this comment

malmans2 Jun 10, 2021

Choose a reason for hiding this comment

oceandie commented Jun 10, 2021

malmans2 Jun 11, 2021

Choose a reason for hiding this comment

oceandie Jun 11, 2021 • edited

Choose a reason for hiding this comment

malmans2 Jun 11, 2021

Choose a reason for hiding this comment

malmans2 Jun 11, 2021

Choose a reason for hiding this comment

oceandie Jun 11, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 7, 2021 •

edited

malmans2 commented Jun 9, 2021 •

edited

malmans2 Jun 10, 2021 •

edited

oceandie Jun 11, 2021 •

edited