Dask landsea masks bugfix #3255

pp-mo · 2019-01-31T17:38:56Z

Fix the problem with landsea-masked PPFields, where the dask compute() calls PPDataProxy.__getitem__, which can then make a nested 'compute()' call for the landsea-mask data.

Closes #3237

Contains content from BLD: Pinned cftime 1.0.1 #3247 , so needs that --or equivalent-- merging, and then a rebase, before merge.
Also needs Test data for landsea-masked PP fields. iris-test-data#52~

pp-mo · 2019-02-01T15:15:23Z

Ping @bjlittle rebased !!

N.B. the commits are a bit of a mess : Please do not merge without squashing !!

pp-mo · 2019-02-04T15:11:49Z

Passes at last 🍾
Mergeable ? 😉

bjlittle · 2019-02-19T14:36:30Z

@pp-mo Can you target the v2.2.x branch instead?

It would be good to bank this fix in a 2.2.1 release 😉

pp-mo · 2019-02-20T10:53:32Z

Re-targetted against v2.2.x, for some reason it wouldn't rebase cleanly, so I skwished + cherry-picked it
( also backed out an unintended change to tests/test_pp_module )
So it had to be force-pushed, but there should be nothing you don't recognise from before.

But all passing so @bjlittle it's go !

bjlittle · 2019-05-16T13:27:28Z

lib/iris/fileformats/pp.py

                (field.lbuser[3] % 1000) == 30:
            land_mask = field

+        apply_landmask = None


@pp-mo Minor point - The PP loading logic is pretty heavy going (always has been). So to help the reader I think it would be make a stronger association that apply_landmask = None is the else of if (field.raw_lbpack // 10 % 10) == 2: ... would you mind doing that?

Also change apply_landmask to land_mask_field, after all that's what it actually is. The use of apply_landmask subtly hints (to me) that it's a boolean, and its not - it's None or a field

bjlittle · 2019-05-16T13:28:22Z

lib/iris/fileformats/pp.py

+                # reference landmask field, so we can't yield them if they
+                # are encountered *before* the landmask.
+                # In that case, defer them, and process them all afterwards at
+                # the end of the file.


@pp-mo Nice clarification, thanks!

bjlittle · 2019-05-16T13:31:23Z

lib/iris/fileformats/pp.py

                continue

-            # Land compressed fields don't have a lbrow and lbnpt.
+            # Landmask-compressed fields don't have an lbrow and lbnpt.


@pp-mo Minor point - Just to match the terminology of your previous comment, could you change this to Land-masked compressed fields..?

bjlittle · 2019-05-16T13:49:31Z

lib/iris/fileformats/pp.py

-        data_shape = (field.lbrow, field.lbnpt)
-        _create_field_data(field, data_shape, land_mask)
+        _create_field_data(field, (field.lbrow, field.lbnpt),
+                           with_landmask_field=apply_landmask)


@pp-mo Could we change this to be:

_create_field_data(field, (field.lbrow, field.lbnpt), land_mask_field=land_mask_field)

bjlittle · 2019-05-16T13:53:38Z

lib/iris/fileformats/pp.py

            field.lbrow, field.lbnpt = mask_shape
-            _create_field_data(field, (field.lbrow, field.lbnpt), land_mask)
+            _create_field_data(field, mask_shape,
+                               with_landmask_field=land_mask)


@pp-mo Again...

_create_field_data(field, mask_shape, land_mask_field=land_mask)

bjlittle · 2019-05-16T13:54:41Z

lib/iris/fileformats/pp.py



-def _create_field_data(field, data_shape, land_mask):
+def _create_field_data(field, data_shape, with_landmask_field=None):


@pp-mo Change with_landmask_field to land_mask_field ... if you agree to the previous comment requests 😉

bjlittle · 2019-05-16T14:14:46Z

lib/iris/fileformats/pp.py


+    If 'with_landmask_field' is passed, it is another field :  The landmask
+    field's data is used as a template for this field's data, determining its
+    size, shape and the locations of valid (non-missing) datapoints.


@pp-mo Minor point - Could we slight reword to something along the lines of...

If 'land_mask_field' is passed (not None), then it is the associated landmask of the `field`. The landmask, which is also a field, is used as a template for the `field` to determine its size, shape and the locations of valid (non-missing) data-points.

bjlittle · 2019-05-16T14:17:21Z

lib/iris/fileformats/pp.py

                                                 loaded_bytes.dtype,
-                                                 field.bmdi, land_mask)
+                                                 field.bmdi,
+                                                 with_landmask_field)


@pp-mo Change to land_mask_field...

bjlittle · 2019-05-16T14:18:37Z

lib/iris/fileformats/pp.py

+                            field.bmdi)
        block_shape = data_shape if 0 not in data_shape else (1, 1)
-        field.data = as_lazy_data(proxy, chunks=block_shape)
+        if with_landmask_field is None:


@pp-mo Change to with_landmask_field to land_mask_field throughout...

bjlittle · 2019-05-16T15:19:25Z

lib/iris/fileformats/pp.py

+                if n_values > 0:
+                    # Note: data field can have excess values, but not fewer.
+                    result[mask] = values[:n_values]
+                return result


@pp-mo Nice 👍

The only part that seems to be missing compare to the functionality of _data_bytes_to_shaped_array is setting the fill_value of the result. Is that still relevant? I'm thinking yes....?

If so, then does the proxy.mdi require to be also passed into the delayed calc_array function?

Sometime back when I was writing this, I had concluded that the fill-value was not respected by dask anyway, so this wasn't necessary.
I think I found that the da.stack operation does not respect fill values, so in most cases we fail to preserve them on load, i.e. the data arrays come out with default fill-values instead of the BMDI value.

But the suggestions feels appropriate + I think does no harm, so I will put it back in.

bjlittle · 2019-05-16T16:18:33Z

@pp-mo Over to you... 😉

pp-mo · 2019-05-17T15:19:23Z

Thanks for taking this on @bjlittle . Brave 🏆 !
I've now tried to address all your comments, and a tiny bit more tidying besides.
Please re-review.

bjlittle · 2019-05-20T09:08:52Z

lib/iris/fileformats/pp.py

    """
    Modifies a field's ``_data`` attribute either by:
-     * converting DeferredArrayBytes into a lazy array,
+     * converting a 'deferred array bytes' tuple into a lazy array,


@pp-mo Yup 👍

bjlittle · 2019-05-20T09:16:13Z

lib/iris/fileformats/pp.py

                (field.lbuser[3] // 1000) == 0 and \
                (field.lbuser[3] % 1000) == 30:
-            land_mask = field
+            land_mask_field = field


@pp-mo We're assuming that the land_mask_field is constant, right? i.e. multiple occurrences are simply duplicates

If that's the case, then we're good... and I'm really hoping that's the case here, otherwise it's going to get complicated quickly 😨

No problem here, discussed to clarify. Always snaps the first land/sea mask.

bjlittle · 2019-05-20T09:19:45Z

@pp-mo Nice one!
Only one last outstanding query, otherwise LGTM 👍

pp-mo · 2019-05-20T09:38:53Z

Magic, thanks @bjlittle !
Huge relief 😅

* Fix landsea-mask data access for UM files. * Review changes. * Fixed some stale comments + unused variables.

pp-mo mentioned this pull request Jan 31, 2019

Test data for landsea-masked PP fields. SciTools/iris-test-data#52

Merged

pp-mo force-pushed the dask_landsea_masks_tempfix branch from 26c5bd2 to d174d3b Compare February 1, 2019 15:14

pp-mo changed the title ~~Dask landsea masks tempfix~~ Dask landsea masks bugfix Feb 5, 2019

pp-mo mentioned this pull request Feb 8, 2019

Problem with data access from PP file loaded cubes and numpy 1.16 #3266

Closed

bjlittle self-assigned this Feb 19, 2019

bjlittle added Type: Bug Release: Patch labels Feb 19, 2019

bjlittle added this to the v2.1.1 milestone Feb 19, 2019

pp-mo changed the base branch from master to v2.2.x February 19, 2019 17:26

SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 19, 2019

pp-mo changed the base branch from v2.2.x to master February 19, 2019 17:27

SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 19, 2019

pp-mo changed the base branch from master to v2.2.x February 19, 2019 17:33

SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 19, 2019

pp-mo force-pushed the dask_landsea_masks_tempfix branch from b177ba7 to 8b03a2d Compare February 19, 2019 17:33

Fix landsea-mask data access for UM files.

b853065

pp-mo force-pushed the dask_landsea_masks_tempfix branch from 8b03a2d to b853065 Compare February 19, 2019 17:45

SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 19, 2019

lbdreyer modified the milestones: v2.1.1, v2.2.1 Feb 22, 2019

pp-mo requested a review from bjlittle February 22, 2019 15:49

bjlittle reviewed May 16, 2019

View reviewed changes

pp-mo added 2 commits May 17, 2019 15:54

Review changes.

918aded

Fixed some stale comments + unused variables.

5864690

bjlittle reviewed May 20, 2019

View reviewed changes

bjlittle merged commit aad43f7 into SciTools:v2.2.x May 20, 2019

lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request May 28, 2019

Dask landsea masks bugfix (SciTools#3255)

929f583

* Fix landsea-mask data access for UM files. * Review changes. * Fixed some stale comments + unused variables.

lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request Jun 3, 2019

Dask landsea masks bugfix (SciTools#3255)

bd8b2a1

* Fix landsea-mask data access for UM files. * Review changes. * Fixed some stale comments + unused variables.

lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request Jun 3, 2019

Dask landsea masks bugfix (SciTools#3255)

859e0c5

* Fix landsea-mask data access for UM files. * Review changes. * Fixed some stale comments + unused variables.

bjlittle mentioned this pull request Jun 5, 2019

Don't nest compute calls #3237

Closed

pp-mo mentioned this pull request Mar 3, 2021

Meshcoord dynamic pp-mo/iris#66

Closed

pp-mo deleted the dask_landsea_masks_tempfix branch March 18, 2022 15:37

trexfeathers mentioned this pull request May 5, 2025

Lockfile updates plus adapt to Dask changes to delayed and internals #6438

Closed



		def _create_field_data(field, data_shape, land_mask):
		def _create_field_data(field, data_shape, with_landmask_field=None):

Dask landsea masks bugfix #3255

Dask landsea masks bugfix #3255

Uh oh!

Conversation

pp-mo commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pp-mo commented Feb 1, 2019

Uh oh!

pp-mo commented Feb 4, 2019

Uh oh!

bjlittle commented Feb 19, 2019

Uh oh!

pp-mo commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle commented May 16, 2019

Uh oh!

pp-mo commented May 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle commented May 20, 2019

Uh oh!

pp-mo commented May 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pp-mo commented Jan 31, 2019 •

edited

Loading

pp-mo commented Feb 20, 2019 •

edited

Loading

bjlittle May 16, 2019 •

edited

Loading

bjlittle May 16, 2019 •

edited

Loading

bjlittle May 16, 2019 •

edited

Loading

bjlittle May 20, 2019 •

edited

Loading