LAMA to Dask migration: Data.concatenate#425
LAMA to Dask migration: Data.concatenate#425sadielbartholomew merged 20 commits intoNCAS-CMS:lama-to-daskfrom
Data.concatenate#425Conversation
sadielbartholomew
changed the title
concatenateData.concatenate
davidhassell
left a comment
davidhassell
left a comment
There was a problem hiding this comment.
Thanks, Sadie - looks good (all wheat, no chaff!). Some suggestions, but only on style and detail.
| # ------------------------------------------------------------ | ||
| return data0 | ||
|
|
||
| def _move_flip_to_partitions(self): |
There was a problem hiding this comment.
We can nuke this LAMA-only method.
There was a problem hiding this comment.
Sure. Do you think there are any / many more to nuke, since if there are I would prefer to do it another PR for separation of concerns? Otherwise for one (or maybe two) I can nuke them here as suggested.
There was a problem hiding this comment.
I don't know, but flake8 should alert us to unused methods. I don't mind if you nuke this here, or there!
| @unittest.skipIf(TEST_DASKIFIED_ONLY, "no attribute '_auxiliary_mask'") | ||
| def test_Data_AUXILIARY_MASK(self): | ||
| def test_Data_concatenate(self): | ||
| if self.test_only and inspect.stack()[0][3] not in self.test_only: |
There was a problem hiding this comment.
We can nuke these "test_only" lines now that the tests are so fast.
There was a problem hiding this comment.
Can I nuke them all (there's OOM 10-100 still here in the test_Data module) in a separate PR along with the LAMA-only methods, e.g. the above, instead of here, to keep the PR nice and self-contained?
There was a problem hiding this comment.
Sure - makes sense (sorry for not thinking of that!)
There was a problem hiding this comment.
Not at all, it's good to raise issues as you spot them :) I've noted to do a follow-on PR (after I have put up the stats and concatenate_data PRs).
Co-authored-by: David Hassell <[email protected]>
davidhassell
left a comment
davidhassell
left a comment
There was a problem hiding this comment.
All good - please merge!
2 participants
Migrating the
Data.concatenatemethod towards #182.We can ignore the CI job checks since they will fail due on
cfdmcompatibility, i.e. for known reasons unrelated to the PR. Locally, the relevant and updatedtest_Data,pypasses.