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

LAMA to Dask migration: Data.concatenate #425

Conversation

sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Jul 25, 2022

Migrating the Data.concatenate method towards #182.

We can ignore the CI job checks since they will fail due on cfdm compatibility, i.e. for known reasons unrelated to the PR. Locally, the relevant and updated test_Data,py passes.

@sadielbartholomew sadielbartholomew added the dask Relating to the use of Dask label Jul 25, 2022
@sadielbartholomew sadielbartholomew marked this pull request as ready for review July 27, 2022 00:18
@sadielbartholomew sadielbartholomew self-assigned this Jul 27, 2022
@sadielbartholomew sadielbartholomew changed the title LAMA to Dask: concatenate LAMA to Dask migration: Data.concatenate Jul 27, 2022
Copy link
Collaborator

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Sadie - looks good (all wheat, no chaff!). Some suggestions, but only on style and detail.

cf/data/data.py Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved
cf/data/data.py Outdated Show resolved Hide resolved

# ------------------------------------------------------------
# Done
# ------------------------------------------------------------
return data0

def _move_flip_to_partitions(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can nuke this LAMA-only method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, but flake8 should alert us to unused methods. I don't mind if you nuke this here, or there!

cf/test/test_Data.py Outdated Show resolved Hide resolved
@@ -977,103 +977,98 @@ def test_Data_cached_arithmetic_units(self):
# Reset
cf.constants.CONSTANTS["FM_THRESHOLD"] = fmt

@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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can nuke these "test_only" lines now that the tests are so fast.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - makes sense (sorry for not thinking of that!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good - please merge!

cf/data/data.py Show resolved Hide resolved
@sadielbartholomew sadielbartholomew merged commit 799d49f into NCAS-CMS:lama-to-dask Aug 2, 2022
@sadielbartholomew sadielbartholomew deleted the lama-to-dask-concatenate branch August 2, 2022 07:02
@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