Migrate Data.transpose from LAMA to Dask
#247
Conversation
|
Of relevance to this PR and to keep an eye on going forward: data-apis/array-api#228 |
|
HI @davidhassell, just 'bumping' this in case it fell off your radar. It would be good to get all outstanding |
cf/data/data.py
Outdated
| @@ -12142,21 +12142,14 @@ def transpose(self, axes=None, inplace=False, i=False): | |||
| ) | |||
| # --- End: if | |||
There was a problem hiding this comment.
Can we ditch all of this checking, as dask duplicates it, and replace it with a try ... except ValueError ... around the actual dask transpose operation?
In [17]: from dask import array as da
In [18]: x = da.from_array([[2, 1]])
In [19]: x.transpose([2, 0])
...
ValueError: ('Unknown dimension', {2})
In [20]: x.transpose([1, 0, -1])
...
ValueError: axes don't match arrayThere was a problem hiding this comment.
Good thinking, I didn't consider that. I imagine so, let me try it out and see...
There was a problem hiding this comment.
OK now I am implementing it, I see we use iaxes for the new d._axes value calculation, so we do need (want) the if/else (as it also short circuits for a trivial case which is good generally) but can cut all but the first line from the else (this might be what you were implying to being with). Unless, that is, there is a clever way to avoid calculating iaxes or the equivalent...
I'm about to push a new commit but if you have thoughts or a new idea relating to this, just let me know and we can tweak it.
There was a problem hiding this comment.
Ah - I see. Given our need to also permute the _axes attribute, I can't see a way other than keeping the existing code. No problem
cf/test/test_Data.py
Outdated
| @@ -859,6 +859,7 @@ def test_Data_squeeze_insert_dimension(self): | |||
| def test_Data___getitem__(self): | |||
| if self.test_only and inspect.stack()[0][3] not in self.test_only: | |||
| return | |||
| # TODODASK - why is there no testing here? | |||
There was a problem hiding this comment.
I don't know! We'll certainly get some in when __getitem__ is daskified.
There was a problem hiding this comment.
... which has now happened (#257)
There was a problem hiding this comment.
Good spot. I'll remove this line. (Given the time since I put up this PR I probably should have done a quick scan to check for anything that could have been updated before tagging you, sorry 🙂)
Co-authored-by: David Hassell <[email protected]>
Co-authored-by: David Hassell <[email protected]>
sadielbartholomew
force-pushed
the
lama-to-dask-4
branch
from
39eff85
to
dd19fa3
Compare
|
As well as adding a few new commits to address the feedback, I have rebased onto |
|
... hence the need to force-push. |
|
Trying via open-close to re-trigger the CI jobs to indicate that the relevant test module passes... |
|
Thanks for your feedback @davidhassell, I believe I have addressed everything now so let me know if any adjustments are required or desired. |
|
Hi @davidhassell, since you are about to go on leave thought I might bump this in case you have a chance to confirm, is it ready to be merged? Next week I will return somewhat to cf-python daskifying work, FYI, though naturally mostly focusing on ES-DOC machine and performance. |
Co-authored-by: David Hassell <[email protected]>
|
Thanks for the nudge. I've made a minor style suggestion, but otherwise good to go! |
|
Thanks for all of the carefully-considered feedback @davidhassell, I have corrected the typo you pointed out in a final commit so will now merge. |
Towards #182, 'daskify' the
Data.transposemethod. This one is fairly trivial (I believe) since there is a Dask intrinsic method totranspose.Note I had to remove the loop and context management over
chunksize in self.chunk_sizesfor the corresponding test, but otherwise I have not touched that test, which now passes, as indicated by the removal of theunittest.skipIfdecorator on the test which now passes locally and the CI will indicate if it passes across the job OS and version matrix.