Skip to content

Honor output range distribution in dash::transform - #398

Open
devreal wants to merge 5 commits into
developmentfrom
bug-386-transform
Open

Honor output range distribution in dash::transform#398
devreal wants to merge 5 commits into
developmentfrom
bug-386-transform

Conversation

@devreal

@devreal devreal commented May 10, 2017

Copy link
Copy Markdown
Member
  1. Implement global-to-global dash::copy, needed for
  2. dash::transform that honors the distribution of the output range for local input ranges, which
  3. now also works with std iterators, .e.g, coming from std::vector

Fixes #386

@bertwesarg bertwesarg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not yet tested.

auto out_it = out_first;
auto in_it = in_first;
while (towrite > 0) {
auto lpos = out_it.lpos();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't the local_size covers the total number of elements in the that unit? What if the unit has multiple local blocks?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That is what I already mentioned multiple times. This approach only works for a single continuous index range. Hence it would be better to use the new range based views interface by @fuchsto . There should be something like dash::chunks(dash::local(...)). Unfortunately I do not have detailed knowledge in this API either.

@fuchsto fuchsto May 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@fmoessbauer @bertwesarg
Yes, almost, it's
dash::blocks(dash::local(view_or_range_expr))
or
dash::local(dash::blocks(view_or_range_expr)).
Containers are valid range expressions, iterators can be converted to ranges using
dash::make_range(it_begin, it_end).

The term chunks was intended for any ad-hoc user-specified decomposition like dash::chunks(dash::stride(10, array)), blocks depend on the pattern. But actually there is no reason to differentiate between those.

dash::for_each_with_index(global_v.begin() + 1, global_v.end(),
[](value_t val, size_t idx){
ASSERT_EQ_U(val, (dash::size() - 1) * 1000 + (idx - 1));
++idx;

@bertwesarg bertwesarg May 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove ++idx.

dart_gptr_t dest_gptr = out_first.dart_gptr();
// Send accumulate message:
trace.enter_state("transform_blocking");
dash::internal::transform_blocking_impl(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unused?

@fuchsto

fuchsto commented Jun 2, 2017

Copy link
Copy Markdown
Member

@devreal I merged your changes into PR #410 to fix this on a "substantial" level using views / ranges, your unit test are especially useful for this.

@devreal

devreal commented Jun 2, 2017

Copy link
Copy Markdown
Member Author

Perfect, thanks a lot!

@devreal devreal modified the milestones: dash-0.3.0, dash-0.4.0 Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants