fixing the cutoff-longitude, ckd-lost-in-NaN problem, enabling CS SN computation #235

MaceKuailv · 2022-06-22T14:41:55Z

#215 : Now when you cut off from 179 to -179 you are cutting out 2 degree instead of 358. The test is also updated
#228 : Fillna in the create tree function, so that ckd tree works on llc-rearranged grid.
#234 : A new function in compute.py to calculate the grid orientation using spherical triangles.

pep8speaks · 2022-06-22T14:42:02Z

Hello @MaceKuailv! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-06 08:25:29 UTC

ThomasHaine · 2022-06-22T16:54:26Z

@MaceKuailv please fix the failing tests. Then @malmans2 please review. Thanks!

codecov · 2022-06-22T19:11:39Z

Codecov Report

Merging #235 (514e150) into master (28b1424) will decrease coverage by 1.15%.
The diff coverage is 37.14%.

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   96.75%   95.59%   -1.16%     
==========================================
  Files          10       10              
  Lines        3570     3634      +64     
  Branches      775      773       -2     
==========================================
+ Hits         3454     3474      +20     
- Misses         67      109      +42     
- Partials       49       51       +2

Flag	Coverage Δ
unittests	`95.59% <37.14%> (-1.16%)`	⬇️

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

Impacted Files	Coverage Δ
oceanspy/compute.py	`97.56% <7.69%> (-1.26%)`	⬇️
oceanspy/utils.py	`78.57% <30.43%> (-18.15%)`	⬇️
oceanspy/_oceandataset.py	`97.59% <100.00%> (+<0.01%)`	⬆️
oceanspy/subsample.py	`98.33% <100.00%> (+0.34%)`	⬆️

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 28b1424...514e150. Read the comment docs.

MaceKuailv · 2022-06-22T20:01:28Z

the conf.py file need to "import oceanspy" which will require import utils which will then "from numba import njit". I have to try except import numba, and if that does not work I need to change njit to a function that reads function f and returns the exact same function f.

malmans2 · 2022-06-23T07:51:15Z

oceanspy/utils.py

+# Jiang being a bit lazy
+import numpy as np
+
+try:
+    from numba import njit
+except ImportError:  # pragma: no cover
+
+    def njit(f):
+        return f


I think it would be a bit cleaner to do something like this:

Suggested change

# Jiang being a bit lazy

import numpy as np

try:

from numba import njit

except ImportError: # pragma: no cover

def njit(f):

return f

import numpy as np

try:

import numba

has_numba = True

except ImportError:

has_numba = False

def conditional_njit(f):

if has_numba:

return numba.njit(f)

return f

I imagine you are seeing quite a bit of improvement using numba. Fell free to use the decorator on other functions as well!

Thanks a lot for the comment! I assume I need to change all the @njit to @conditional_njit in the rest of the code. Is that correct?

correct! also, I haven't tested the snippet above - hopefully it works fine

No worries. I am going to test it

malmans2 · 2022-06-23T08:09:54Z

oceanspy/_oceandataset.py

+        x_stack = x.stack(points=x.dims).fillna(rid_value).values
+        y_stack = y.stack(points=y.dims).fillna(rid_value).values
+        z_stack = z.stack(points=z.dims).fillna(rid_value).values


Does it work if you use .data instead of .values? That way dask arrays are not loaded into memory.

I haven't tested that. But since the very next step is to feed the whole thing into scipy, I think laziness is not really necessary here. I believe it is also values in the original code

Let's use .data if it works. It wasn't available when we first started working on oceanspy and it is possible that SciPy is/will be able to use dask laziness

malmans2 · 2022-06-23T08:16:07Z

oceanspy/utils.py

+    read the od grid things into numpy arrays
+    stored in this module
+    """
+    global Z, Zl, dZ, dZl, dXC, dYC, dXG, dYG, XC, XG, YC, YG, CS, SN, tree, ts


We should try to avoid global variables as much as possible (or it would be better to use python's cache decorators)

That's very true. This is actually a temporary solution. My plan is to put the functions you see here into the _oceandataset as methods, and those global variables in the util module will be in the class oceandataset. But oceandataset is very fundamental to a lot of the operations, so I want to be more careful when making changes to it.

…ata in create_tree(I will look at other functions at some later time)

Mikejmnez · 2022-06-29T15:56:59Z

@MaceKuailv the variables CS and SN are not available on the dataset after reading it with oceanspy (see issue #234 ) because in the intake catalog, I decided to drop them - I didn't think they were relevant to users but I see I was mistaken. You can include these variables by simply commenting the line 726 in /sciserver_catalogs/catalog_xarray.yaml the line 726. You can still keep the function you created for calculating these variables (and perhaps write a test to compare) as I think it still might be useful.

Mikejmnez · 2022-06-29T20:48:44Z

oceanspy/compute.py

@@ -2833,6 +2834,43 @@ def salt_budget(od):
    return _ospy.OceanDataset(ds).dataset


+def find_cs_sn(thetaA, phiA, thetaB, phiB):
+    """
+    theta is the angle


You should improve the description of this function. What does it do? Input and outputs.

Mikejmnez · 2022-06-29T20:55:52Z

oceanspy/utils.py

@@ -18,6 +18,31 @@
 except ImportError:  # pragma: no cover
    pass

+import numpy as np


do instead

import numpy as _np

this is consistent with the rest of oceanspy.

Mikejmnez · 2022-06-29T20:59:05Z

oceanspy/utils.py

+        CS = np.array(od._ds.CS).astype("float32")
+        SN = np.array(od._ds.SN).astype("float32")
+        tree = od.create_tree(gridtype)
+    if gridtype == "C":


This can create confusion, since oceanspy is only compatible with c-grids. Instead of gridtype, use something like gridloc (as in location) or something. We don't want to suggest that these are different types of grids...

@MaceKuailv Please make these fixes and @asiddi24 will investigate the failing 3.10 build (also for #237).

MaceKuailv · 2022-07-06T06:57:13Z

Ready to be merged when the _xe problem is fixed

malmans2 · 2022-07-06T08:00:31Z

Ready to be merged when the _xe problem is fixed

See #238

malmans2 · 2022-07-06T08:16:39Z

If you trigger the CI should be all good now

ThomasHaine

Looks good to me.

MaceKuailv · 2022-07-06T12:46:45Z

Noice

MaceKuailv and others added 7 commits June 22, 2022 10:04

support wrap around function and coordnate translation

8004fa2

Merge branch 'hainegroup:master' into master

4df9e8d

fix longitude issue in cutoff

08e12d4

add the test to see if the cutoff find the correct longitude range

7e39720

Merge branch 'master' of github.com:MaceKuailv/oceanspy

f4997a6

Fix the issue with CKD tree with nan

ee6d803

add a function that computes cos and sin of the grid based on lat-lon

8c6aad9

MaceKuailv requested review from malmans2, renskegelderloos, ThomasHaine, asiddi24 and Mikejmnez as code owners June 22, 2022 14:41

MaceKuailv added 2 commits June 22, 2022 18:57

minor fix of bug and style issues

bb5732e

minor change

3d8fae8

MaceKuailv added 2 commits June 22, 2022 19:24

add try except to numba import

f6d7cd3

turn njit magic into a function if not able to import numba

8a3d9ad

malmans2 reviewed Jun 23, 2022

View reviewed changes

MaceKuailv added 3 commits June 23, 2022 14:28

changes Mattia suggested: cleaner version of njit import; values to d…

cbbd942

…ata in create_tree(I will look at other functions at some later time)

forgot auto style check

c416c51

add a comment about why the test failed the 3.10 test

614f693

Mikejmnez reviewed Jun 29, 2022

View reviewed changes

ThomasHaine mentioned this pull request Jul 5, 2022

Rename master to main #216

Closed

MaceKuailv added 3 commits July 6, 2022 05:19

moved the find local coord functions to elsewhere

bef21df

added a bit api reference

09605d9

some more comments

7fc140a

changed the comments a bit to trigger code check

514e150

ThomasHaine approved these changes Jul 6, 2022

View reviewed changes

ThomasHaine merged commit b8eb69a into hainegroup:master Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing the cutoff-longitude, ckd-lost-in-NaN problem, enabling CS SN computation #235

fixing the cutoff-longitude, ckd-lost-in-NaN problem, enabling CS SN computation #235

MaceKuailv commented Jun 22, 2022

pep8speaks commented Jun 22, 2022 •

edited

Loading

ThomasHaine commented Jun 22, 2022

codecov bot commented Jun 22, 2022 •

edited

Loading

MaceKuailv commented Jun 22, 2022

malmans2 Jun 23, 2022 •

edited

Loading

MaceKuailv Jun 23, 2022

malmans2 Jun 23, 2022

MaceKuailv Jun 23, 2022

malmans2 Jun 23, 2022

MaceKuailv Jun 23, 2022

malmans2 Jun 23, 2022

malmans2 Jun 23, 2022

MaceKuailv Jun 23, 2022

Mikejmnez commented Jun 29, 2022

Mikejmnez Jun 29, 2022 •

edited

Loading

Mikejmnez Jun 29, 2022

Mikejmnez Jun 29, 2022 •

edited

Loading

ThomasHaine Jul 5, 2022

MaceKuailv commented Jul 6, 2022

malmans2 commented Jul 6, 2022

malmans2 commented Jul 6, 2022

ThomasHaine left a comment

MaceKuailv commented Jul 6, 2022

fixing the cutoff-longitude, ckd-lost-in-NaN problem, enabling CS SN computation #235

fixing the cutoff-longitude, ckd-lost-in-NaN problem, enabling CS SN computation #235

Conversation

MaceKuailv commented Jun 22, 2022

pep8speaks commented Jun 22, 2022 • edited Loading

Comment last updated at 2022-07-06 08:25:29 UTC

ThomasHaine commented Jun 22, 2022

codecov bot commented Jun 22, 2022 • edited Loading

Codecov Report

MaceKuailv commented Jun 22, 2022

malmans2 Jun 23, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikejmnez commented Jun 29, 2022

Mikejmnez Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikejmnez Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaceKuailv commented Jul 6, 2022

malmans2 commented Jul 6, 2022

malmans2 commented Jul 6, 2022

ThomasHaine left a comment

Choose a reason for hiding this comment

MaceKuailv commented Jul 6, 2022

pep8speaks commented Jun 22, 2022 •

edited

Loading

codecov bot commented Jun 22, 2022 •

edited

Loading

malmans2 Jun 23, 2022 •

edited

Loading

Mikejmnez Jun 29, 2022 •

edited

Loading

Mikejmnez Jun 29, 2022 •

edited

Loading