Array algebra iter const #445
Conversation
… empty points when unionLattices and intersectLattices are called
|
I am going to merge this anyways since it is needed to fix a benchmarking bug. Hopefully @RawnH can review it later. |
| // Append all combinations of the merge points of a and b | ||
| auto sorted_apoint = left.points(); | ||
| auto sorted_bpoint = right.points(); | ||
| std::sort(sorted_apoint.begin(), sorted_apoint.end(), pointSorter); |
There was a problem hiding this comment.
Aren't the lattice points already sorted? Why do we need to sort again here?
There was a problem hiding this comment.
With my new algorithmic changes there were differences in the ordering of points causing tests-merge_latticce.cpp to fail in two tests even though the lattices were the same. It permuted some of the points on the same-level. This was the easiest way for me to fix it, but I'm also open to other suggestions.
There was a problem hiding this comment.
Oh I remember dealing with that. Seems like a good solution to me.
| auto bpoint_set = bpoint.tensorRegion(); | ||
|
|
||
| for (auto& it : apoint_set) { | ||
| if (!std::count(bpoint_set.begin(), bpoint_set.end(), it) && |
There was a problem hiding this comment.
Low priority, but can these use the taco::util::contains instead?
There was a problem hiding this comment.
Sure, the std::count() online was the preferred method, but taco::util::contains makes more sense to the programmer reading the code.
| auto bpoint_set = bpoint.tensorRegion(); | ||
|
|
||
| for (auto& it : apoint_set) { | ||
| if (!std::count(bpoint_set.begin(), bpoint_set.end(), it) && |
There was a problem hiding this comment.
Same question as above about using util::contains
There was a problem hiding this comment.
Will change this and push directly to array_algebra when I have time. Same as above.
| auto apoints = a.points(); | ||
| auto bpoints = b.points(); | ||
| struct pointSort { | ||
| bool operator()(const MergePoint& a, const MergePoint& b) { |
There was a problem hiding this comment.
Same question about sorting. I think the MergeLattice's points should always be sorted so it seems strange that we need to sort before a comparison
There was a problem hiding this comment.
See above. You sorted before in the original unionLattices(...) anyways when taking the cartesian product so I just took the code from there.
There was a problem hiding this comment.
I see, I was just wondering why a sort inside one of the MergeLattice operators since the points should always be sorted. But as above, I also ran into the issue of points on the same level being permuted.
I think this is fine.
| { | ||
| vector<Iterator> result; | ||
|
|
||
| // Remove all but one of the dense iterators, which are all the same. |
There was a problem hiding this comment.
We are removing all of the dimension iterators right? Not all but one?
There was a problem hiding this comment.
Yea. This was a comment I copied from your deduplicateDImensionIterators(...) function. This comment is wrong, but the code should remove all dense iterators. I will fix this in the same commit when I change the code to taco::util::contains
The code fixes to the array algebra iteration lattice construction algorithm.
It is now passing all of the tests in
make testand furthermore is creating the correct output for many other fused iteration spaces being tested intensor-compiler/array-programming-benchmarks.I would like to add those fused benchmarks in as tests to
tests-merge_lattice.cppandtests-lower.cppbut will do that after 4/16/20