Skip to content

Fix mapchunks and add tests for it#695

Open
alecarraro wants to merge 2 commits intoJuliaParallel:masterfrom
alecarraro:alecarraro/476
Open

Fix mapchunks and add tests for it#695
alecarraro wants to merge 2 commits intoJuliaParallel:masterfrom
alecarraro:alecarraro/476

Conversation

@alecarraro
Copy link
Copy Markdown

Hi, this PR proposes a fix for (#476). Specifically I implemented the following changes:

  1. mapchunks now uses the correct DArray constructor
  2. I switched to Dagger.spawn instead of fetch/collect, so mapchunks integrates with Dagger’s scheduler. I also set Options(meta=false) explicitely to highlight that f gets the materialized chunk data but I guess it might not be necessary.
  3. For type inference, I added a _mapchunks_eltype helper: it looks at chunktype when available, promotes across chunks, and falls back to Any if type inference is not possible. I think an other option could be to use promote_op(f, chunktype(chunk)), but then it should be documented that it fails if the chunk has heterogeneous types. What do you think?
  4. Added some tests. The testset for mapchunks takes 50% longer than the testset for map on my computer. Lmk if you'd prefer it to keep it lighter

Also, the function mapchunks is undocumented at the moment. I would be happy to fix that as well, but it makes more sense to do it after a design is finalized. Since this issue has been around for a while, I guess there might be some prior context I’m missing (i.e planned API changes). Happy to adjust if that's the case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant