Skip to content

Commit

Permalink
Implement reducedim using cartesian
Browse files Browse the repository at this point in the history
  • Loading branch information
@timholy
timholy committed Jan 13, 2014
1 parent 72c357f commit ea7987f
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 388 deletions.
Loading

7 comments on commit ea7987f

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

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

@timholy I just found that these changes break the codes in Distributions.jl by removing methods like sum!(dst, a, region) and maximum!(dst, a, region) etc ...

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

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

I see ... it relies on the shape of dst to determine which dimension to reduce.

@timholy
Copy link
Member Author

Choose a reason for hiding this comment

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

Shoot, sorry about that. But yes, once you've created dst you don't really need region anymore. Will you fix this by changing Distributions.jl, or do you want me to add versions that just drop the region argument?

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

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

I already made changes accordingly in Distributions.jl. No worry about that.

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of the interface, I sometimes wanted to write things like:

v = Array(T, n);
...
a = rand(m, n);
sum!(v, a, 1)
...
return X(v)  # X is a constructor that takes a vector as input

Now, I will have to write

v = Array(T, 1, n);
...
a = rand(m, n);
sum!(fill!(v, 0), a);
...
return X(vec(v));

Not a big deal though.

@timholy
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see you already noticed the need to fill!. I wondered if that was a bug or feature. An interesting thing about not having sum! do the fill itself is that now you can accumulate across multiple inputs a. I thought that was kinda cool, so I didn't have sum! call fill!.

maximum!, etc, all work similarly, of course.

And of course, none of this is exported anyway :-).

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

While this behavior for sum! and maximum! seems useful, it is a bit surprising. How about a fill::Bool=true keyword option? I.e. default to filling the accumulator array with zeros before doing the reduction, but allow fill=false to not do that.

Please sign in to comment.