Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in previously-daskified Data.flip #329

Merged

Conversation

sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Feb 23, 2022

Fix a bug in Data.flip, exposed with the following example, which was lifted from the start of the test_Data_BINARY_AND_UNARY_OPERATORS test, which is where the bug was noticed:

a = np.arange(3 * 4 * 5).reshape(3, 4, 5) + 1.0
a0 = a[(slice(None, None, -1),) * a.ndim]
d = cf.Data(a0)

d.flip()

which, on the present lama-to-dask branch, runs into:

~/cf-python/cf/data/data.py in <listcomp>(.0)
  10958 
  10959         index = [
> 10960             slice(None, None, -1) if i in axes else slice(None) for i in iaxes
  10961         ]
  10962 

TypeError: argument of type 'NoneType' is not iterable

though it is not just a case of converting the axes into a sequence if an integer value was provided (both are permitted as values for that keyword), since there is a further issue in the logic for the creation of the indices list index by that list comprehension that is, for example, revealed in the failing unit test once it has been unskipped (and with some old syntax converted so it runs to pass/fail rather than error) since that test method appears to have been missed to be reinstate despite flip being marked as migrated for #182.

Suggested solution context

With a simple adjustment to ensure axes is a sequence, namely using:

        if isinstance(axes, int):
            axes = [axes,]
        index = [
            slice(None, None, -1) if i in axes else slice(None)
            for i in iaxes
        ]

the indices were (as far as the initial unit test failure):

test_Data_flip (__main__.DataTest) ...
RESULTING INDEX IS [slice(None, None, -1)] IAXES (0,) AXES [0]
RESULTING INDEX IS [slice(None, None, -1)] IAXES (1,) AXES [1]
RESULTING INDEX IS [slice(None, None, -1), slice(None, None, -1)] IAXES (0, 1) AXES [0, 1]
FAIL

which don't look appropriate, where the printed variables result from the following print calls:

diff --git a/cf/data/data.py b/cf/data/data.py
index 5cc6368de..b58b1a550 100644
--- a/cf/data/data.py
+++ b/cf/data/data.py
@@ -10964,11 +10964,12 @@ class Data(Container, cfdm.Data, DataClassDeprecationsMixin):
 
+        print("RESULTING INDEX IS", index, "IAXES", iaxes, "AXES", axes)
         dx = d._get_dask()
         dx = dx[tuple(index)]
         d._set_dask(dx, reset_mask_hardness=False)

With the change to the list comprehension suggested in this PR, arrived at by taking a look at the current/LAMA code defining Data.flip master, the unit test passes and a resulting print-out of the index variable with the axes used in each test case, as above, is instead something which looks right, namely:

test_Data_flip (__main__.DataTest) ...
RESULTING INDEX IS [slice(None, None, -1), slice(None, None, None)] IAXES (0,) AXES 0
RESULTING INDEX IS [slice(None, None, None), slice(None, None, -1)] IAXES (1,) AXES 1
RESULTING INDEX IS [slice(None, None, -1), slice(None, None, -1)] IAXES (0, 1) AXES [0, 1]
RESULTING INDEX IS [slice(None, None, None), slice(None, None, None), slice(None, None, -1)] IAXES (2,) AXES 2
RESULTING INDEX IS [slice(None, None, None), slice(None, None, -1), slice(None, None, None)] IAXES (1,) AXES 1
RESULTING INDEX IS [slice(None, None, -1), slice(None, None, None), slice(None, None, None)] IAXES (0,) AXES 0``

@sadielbartholomew sadielbartholomew self-assigned this Feb 23, 2022
@sadielbartholomew sadielbartholomew changed the title Fix to bug in previously-daskified Data.flip Fix bugs in previously-daskified Data.flip Feb 24, 2022
@sadielbartholomew sadielbartholomew marked this pull request as draft February 24, 2022 02:13
@sadielbartholomew sadielbartholomew marked this pull request as ready for review February 24, 2022 03:00
@sadielbartholomew sadielbartholomew marked this pull request as draft February 24, 2022 03:24
@sadielbartholomew sadielbartholomew changed the title Fix bugs in previously-daskified Data.flip Fix bug in previously-daskified Data.flip Feb 24, 2022
@sadielbartholomew sadielbartholomew marked this pull request as ready for review February 24, 2022 20:15
cf/data/data.py Outdated Show resolved Hide resolved
@sadielbartholomew
Copy link
Member Author

Hi David, your feedback should be addressed now. Please let me know if we're good to merge.

@davidhassell
Copy link
Collaborator

Great - please merge.

@sadielbartholomew sadielbartholomew merged commit 08cab16 into NCAS-CMS:lama-to-dask Feb 28, 2022
@sadielbartholomew sadielbartholomew deleted the daskified-flip-fix branch February 28, 2022 14:39
@davidhassell davidhassell added the dask Relating to the use of Dask label Feb 28, 2022
@davidhassell davidhassell added this to the 3.14.0 milestone Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dask Relating to the use of Dask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants