Region: explicit W/E/S/N bounds and cyclic longitude support; Longitude class and longitude module #266

spencerahill · 2018-04-23T18:10:40Z

Closes #229

Still need to implement the cyclic longitudes logic we converged on in #229; the new logic that's currently in region.py that touches this will be updated, as will the related tests.

Still also need tests and to finish implementation of support for non-aospy data. But wanted to open this PR to get a stake in the ground.

@spencerkclark running the tests on my local machine, the tutorial notebook test failed because I didn't update the Region definitions in the notebook to the new call signature. But now I can't remember how we generated the tutorial notebook. Do you recall? I could manipulate the .ipynb file directly, but this seems fraught.

stickler-ci · 2018-04-23T18:10:53Z

aospy/test/test_region.py

@@ -3,7 +3,7 @@
 import xarray as xr

 from aospy import Region
-from aospy.region import _get_land_mask
+from aospy.region import _arr_to_0360_lon, _get_land_mask, _lon_to_0360


F401 'aospy.region._arr_to_0360_lon' imported but unused
F401 'aospy.region._lon_to_0360' imported but unused

stickler-ci · 2018-04-23T18:10:53Z

aospy/test/test_region.py

+# def test_mask_var_retain_orig_lons(data_for_reg_calcs, lons_orig,
+#                                    reg_lon_bounds):
+#     # Each 'reg_lon_bounds' was chosen so that, with the corresponding
+#     # 'lons_orig', the result will match the 'expected_data' (for convenience)


E501 line too long (80 > 79 characters)

spencerkclark · 2018-04-23T18:44:15Z

But now I can't remember how we generated the tutorial notebook. Do you recall? I could manipulate the .ipynb file directly, but this seems fraught.

Yeah don't edit the the .ipynb file directly :). The easiest thing is to just open it in Jupyter, make the changes, and save it.

stickler-ci · 2018-04-24T00:20:47Z

aospy/test/test_region.py

@@ -3,7 +3,12 @@
 import xarray as xr

 from aospy import Region
-from aospy.region import _get_land_mask
+from aospy.region import (


F401 'aospy.region._lon_to_0360' imported but unused
F401 'aospy.region._arr_to_0360_lon' imported but unused

spencerahill · 2018-04-24T01:02:06Z

The calc test suite is now failing across the board due to a problem with the tearDown, e.g. here.

I don't see an obvious way that what I introduced caused this, although this is the first time we're seeing it. I just added a try/except guard to see if that solves it.

spencerahill · 2018-04-24T01:29:39Z

I just added a try/except guard to see if that solves it.

Hmm, now these have been replaced by even less intuitive ones, e.g. here

It seems like the example objects within the CI environments have somehow been corrupted. Maybe due to the change in Region call signature? In contrast, these tests are all passing on my local machine.

spencerkclark · 2018-04-24T01:38:55Z

Hmm, now these have been replaced by even less intuitive ones, e.g. here

I think those were always there -- see L1828-L1873 in the old build for example. The tear down errors were just side effects of the results never being computed/saved to files.

Xarray recently released a new version (version 0.10.3); which version are you on locally?

spencerahill · 2018-04-24T01:44:59Z

Xarray recently released a new version (version 0.10.3); which version are you on locally?

Good call. I was on 10.2; after updating to 10.3 I can reproduce. I'll open a separate issue to track this.

spencerkclark

I still need to look at the code, but I read through the docstrings (which are in general very nice!) and noticed a few minor things that I didn't want to forget.

spencerkclark · 2018-04-27T00:50:34Z

aospy/region.py

-             boundaries are more complicated than a single lat-lon rectangle,
-             use `mask_bounds` instead.
+        west_bound, east_bound, south_bound, north_bound : scalar, optional
+             The western, eastern southern, and northern boundaries,


Typo: add a comma between "eastern" and "southern."

spencerkclark · 2018-04-27T00:58:17Z

aospy/region.py

-        (i.e.  it crosses the Prime Meridian for data with longitudes spanning
-        0 to 360), it must be defined as the union of two sections extending to
-        the east and to the west of the Prime Meridian.
+        Note that longitudes spanning (-180, 180), (0, 360), or any other range


Should (0, 360) be (180, 540) to correspond with the example after the colon?

spencerkclark · 2018-04-27T00:59:44Z

aospy/region.py

-        >>> globe = Region(name='globe', lat_bounds=(-90, 90),
-        ...                lon_bounds=(0, 360), do_land_mask=False)
+        >>> globe = Region(name='globe', west_bound=0, east_bound=360,
+        ...                south_bound=0, north_bound=360, do_land_mask=False)


I'm guessing for this example south_bound=-90 and north_bound=90 should be used.

spencerahill · 2018-04-27T01:11:23Z

I still need to look at the code, but I read through the docstrings (which are in general very nice!) and noticed a few minor things that I didn't want to forget.

Thanks! FYI I've realized after this commit that the logic I implemented is incomplete (i.e. wrong)...I'm working on a better solution and will ping you when it's ready for a proper review.

stickler-ci · 2018-05-01T16:01:39Z

aospy/utils/__init__.py

@@ -1,6 +1,6 @@
 """Subpackage comprising various utility functions used elsewhere in aospy."""
 from . import io
+from . import longitude


F401 '.longitude' imported but unused

stickler-ci · 2018-05-01T16:01:40Z

aospy/utils/__init__.py

@@ -1,6 +1,6 @@
 """Subpackage comprising various utility functions used elsewhere in aospy."""
 from . import io
+from . import longitude
+from .longitude import Longitude


F401 '.longitude.Longitude' imported but unused

spencerahill · 2018-05-01T23:24:33Z

OK, c.f. #272, I have re-done the logic using the new utils.longitude module and Longitude class. Remaining items to address for my own organizational purposes:

Make a decision about lon_cyclic ~~(actually I think I've worked this out; new commit forthcoming)~~
Add Longitude to main docs
Double-check docs for Region
What's new

spencerahill · 2018-05-01T23:49:18Z

C.f. my comment #267 (comment), I think after all that dealing with non-cyclic longitudes is probably outside of the scope of this PR (despite that being partly the original motivation). Frankly, I am almost out of spare cycles for the next few weeks, and I want to get what's here so far in.

So we would return to the cyclic points in conjunction with how we treat region boundaries in a future PR. @spencerkclark does that sound OK to you? Sorry that I turned this one into such a long and winding road.

Either way, I'd appreciate a review of this PR when you get the chance. Thanks!

spencerkclark · 2018-05-01T23:56:52Z

So we would return to the cyclic points in conjunction with how we treat region boundaries in a future PR. @spencerkclark does that sound OK to you?

Totally fine! I'll try and give things a review over the next few days.

spencerkclark

Thanks @spencerahill, this is going to be a big upgrade for our region handling. I have mostly cosmetic comments, the one exception being one on the comparison of Longitude objects to DataArrays.

spencerkclark · 2018-05-02T11:53:25Z

aospy/utils/longitude.py

+        if isinstance(other, Longitude):
+            cond = (self.hemisphere == other.hemisphere and
+                    self.longitude == other.longitude)
+            if cond:


I believe this if-else block is equivalent to simply returning cond.

spencerkclark · 2018-05-02T12:12:00Z

aospy/test/test_utils_longitude.py

+     (Longitude(0), Longitude(0)),
+     (Longitude('0E'), 0),
+     (Longitude('0E'), 720),
+     (Longitude('0E'), [0, 720])])


Here and for the other operators, should we test arrays on either the right or left side of the operator? For example:

@pytest.mark.parametrize( ('obj1', 'obj2'), [(Longitude('100W'), Longitude('100W')), ... (Longitude('0E'), [0, 720]), ([0, 720], Longitude('0E'))])

It would also be good to add test coverage for use with DataArrays.

spencerkclark · 2018-05-02T12:31:47Z

aospy/utils/longitude.py

+            else:
+                return False
+        else:
+            return np.equal(other, self)


Here and in the other operators we need special logic if other is a DataArray; as written this currently doesn't work:

In [1]: import xarray as xr In [2]: from longitude import Longitude In [3]: Longitude(0.) == xr.DataArray([0., 720]) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-3-85150267fe77> in <module>() ----> 1 Longitude(0.) == xr.DataArray([0., 720]) /Users/spencerclark/Downloads/lon_sah2/longitude.py in func_other_to_lon(self, other) 72 def other_to_lon(func): 73 def func_other_to_lon(self, other): ---> 74 return func(self, _maybe_cast_to_lon(other)) 75 return func_other_to_lon 76 /Users/spencerclark/Downloads/lon_sah2/longitude.py in __eq__(self, other) 136 return False 137 else: --> 138 return np.equal(other, self) 139 140 @other_to_lon TypeError: operand type(s) all returned NotImplemented from __array_ufunc__(<ufunc 'equal'>, '__call__', <xarray.DataArray (dim_0: 2)> array([ 0., 720.]) Dimensions without coordinates: dim_0, Longitude('0.0E')): 'DataArray', 'Longitude'

I think the following should fix things:

if isinstance(other, xr.DataArray): return xr.apply_ufunc(np.equal, other, self) else: return np.equal(other, self)

spencerkclark · 2018-05-02T12:50:58Z

aospy/region.py

    def __str__(self):
        return 'Geographical region "' + self.name + '"'

-    __repr__ = __str__
+    def _make_mask(self, data, lon_str=LON_STR, lat_str=LAT_STR):


What is the reasoning behind having this function take lon_str and lat_str as arguments? As part of the data-loading process don't we automatically rename the longitude and latitude coordinates LON_STR and LAT_STR? Is it that you are envisioning using these functions outside of aospy?

That's right. This gets called by mask_var, which in turn gets called by ts and its siblings, which I want to make available to non-aospy data.

spencerkclark · 2018-05-02T13:01:16Z

aospy/region.py

+                                     "south, north).  Value given: "
+                                     "{}".format(rect_bounds))
+                else:
+                    lons = _lon_bounds_to_lon_obj(rect_bounds[:2])


Even though it's an extra line, I have a slight preference for using tuple unpacking to extract the bounds here before converting to Longitude objects (for clarity):

west_bound, east_bound, south_bound, north_bound = rect_bounds lons = _lon_bounds_to_lon_obj([west_bound, east_bound]) bounds.append(tuple(lons + [south_bound, north_bound]))

I'm going to switch to a namedtuple, which will make this logic cleaner

spencerkclark · 2018-05-02T13:11:20Z

aospy/test/test_region.py

+    region = Region(name='test', mask_bounds=bounds_in)
+    assert isinstance(region.mask_bounds, tuple)
+    assert len(region.mask_bounds) == 2
+    for n, bounds in enumerate(region.mask_bounds):


Slight preference for using zip and tuple unpacking here rather than enumerate and indexing the bounds_in list/tuples:

for (west, east, south, north), bounds in zip(bounds_in, region.mask_bounds): assert isinstance(bounds, tuple) assert np.all(bounds == (Longitude(west), Longitude(east), south, north))

spencerkclark · 2018-05-02T13:13:21Z

aospy/test/test_region.py

+                            (1, 2, 3)])
+
+
+# def test_make_mask_latlon_rect(data_for_reg_calcs):


Is there a reason behind these commented-out tests?

Sorry, forgot to remove these

spencerahill · 2018-05-02T18:42:45Z

@spencerkclark thanks, your comments all look good. Will address + flesh out the docs.

stickler-ci · 2018-05-02T20:04:59Z

aospy/region.py

+    SFC_AREA_STR,
+    YEAR_STR
+)
+from .utils.longitude import Longitude, _maybe_cast_to_lon


F401 '.utils.longitude.Longitude' imported but unused

spencerahill · 2018-05-02T21:40:41Z

OK, I think this is ready to go. Will merge once tests pass (EDIT and add a What's New). Thanks @spencerkclark for the invaluable help.

WIP Region: explicit W/E/S/N bounds; begin cyclic lon fixes

2b8de31

stickler-ci reviewed Apr 23, 2018

View reviewed changes

Spencer Hill added 2 commits April 23, 2018 17:15

Fix logic for non-aospy names; add tests

b9e39f3

Update Region calls in tutorial Notebook

d61c2d6

stickler-ci reviewed Apr 24, 2018

View reviewed changes

Attempt to fix CI errors on calc tests teardown

5ed9758

spencerahill mentioned this pull request Apr 25, 2018

Failing test_calc_basic tests after updating xarray from 10.2 to 10.3 #268

Closed

Spencer Hill added 4 commits April 24, 2018 17:24

Shift data's lons to start at Region's west_bound

b7b5568

Initial add of lon_cyclic to Model

fac34d0

Merge branch 'develop' into fix-region-lon-bounds

ed88739

Add pytest catch to .gitignore

7c3c853

spencerkclark reviewed Apr 27, 2018

View reviewed changes

Merge branch 'develop' into fix-region-lon-bounds

9e376f0

spencerahill mentioned this pull request Apr 27, 2018

Proposal: utils.longitude module and Longitude class #272

Closed

Spencer Hill added 2 commits May 1, 2018 08:58

Add utils.longitude and Longitude class

c512670

Use Longitude for Region bounds and masking

ea69284

stickler-ci reviewed May 1, 2018

View reviewed changes

Re-include __all__ in utils.__init__ to satisfy stickler-ci

3c09643

spencerahill mentioned this pull request May 1, 2018

Region masks compares region edges to cell centers #267

Open

spencerkclark reviewed May 2, 2018

View reviewed changes

Address review comments

4453721

stickler-ci reviewed May 2, 2018

View reviewed changes

Spencer Hill added 3 commits May 2, 2018 13:05

remove lon_cyclic from Model; will be added in future PR

4c0a5ce

Add longitude documentation

a7da345

Add longitude to API docs page

c0ede00

what's new entries

ea54566

spencerahill merged commit 7162010 into develop May 2, 2018

spencerahill deleted the fix-region-lon-bounds branch May 2, 2018 22:02

spencerahill changed the title ~~WIP Region: explicit W/E/S/N bounds and cyclic longitude support~~ Region: explicit W/E/S/N bounds and cyclic longitude support; Longitude class and longitude module May 14, 2018

This was referenced Nov 3, 2018

Update lon and lat bounds of example in our docs #294

Closed

Data with coordinate variables (lat, lon, etc.) with names not included in internal_names #293

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Region: explicit W/E/S/N bounds and cyclic longitude support; Longitude class and longitude module #266

Region: explicit W/E/S/N bounds and cyclic longitude support; Longitude class and longitude module #266

spencerahill commented Apr 23, 2018

stickler-ci Apr 23, 2018

stickler-ci Apr 23, 2018

spencerkclark commented Apr 23, 2018

stickler-ci Apr 24, 2018

spencerahill commented Apr 24, 2018

spencerahill commented Apr 24, 2018 •

edited

Loading

spencerkclark commented Apr 24, 2018

spencerahill commented Apr 24, 2018

spencerkclark left a comment

spencerkclark Apr 27, 2018

spencerkclark Apr 27, 2018

spencerkclark Apr 27, 2018

spencerahill commented Apr 27, 2018

stickler-ci May 1, 2018

stickler-ci May 1, 2018

spencerahill commented May 1, 2018 •

edited

Loading

spencerahill commented May 1, 2018

spencerkclark commented May 1, 2018

spencerkclark left a comment

spencerkclark May 2, 2018

spencerkclark May 2, 2018

spencerkclark May 2, 2018

spencerkclark May 2, 2018

spencerkclark May 2, 2018

spencerahill May 2, 2018

spencerkclark May 2, 2018

spencerahill May 2, 2018

spencerkclark May 2, 2018

spencerkclark May 2, 2018

spencerahill May 2, 2018

spencerahill commented May 2, 2018

stickler-ci May 2, 2018

spencerahill commented May 2, 2018 •

edited

Loading

		(1, 2, 3)])


		# def test_make_mask_latlon_rect(data_for_reg_calcs):

Region: explicit W/E/S/N bounds and cyclic longitude support; Longitude class and longitude module #266

Region: explicit W/E/S/N bounds and cyclic longitude support; Longitude class and longitude module #266

Conversation

spencerahill commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerkclark commented Apr 23, 2018

Choose a reason for hiding this comment

spencerahill commented Apr 24, 2018

spencerahill commented Apr 24, 2018 • edited Loading

spencerkclark commented Apr 24, 2018

spencerahill commented Apr 24, 2018

spencerkclark left a 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

spencerahill commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerahill commented May 1, 2018 • edited Loading

spencerahill commented May 1, 2018

spencerkclark commented May 1, 2018

spencerkclark left a 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

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

spencerahill commented May 2, 2018

Choose a reason for hiding this comment

spencerahill commented May 2, 2018 • edited Loading

spencerahill commented Apr 24, 2018 •

edited

Loading

spencerahill commented May 1, 2018 •

edited

Loading

spencerahill commented May 2, 2018 •

edited

Loading