Skip to content

Commit 539a994

Browse files
anyzelmananhansson
anyzelman
authored and
anhansson
committed
533 out of place set does not clear the output vector (#96)
The `grb::set` did not follow out-of-place semantics, while the unit test designed to catch that was bugged. Additionally, the label propagation smoke test relied on the older in-place semantics. This MR makes sure the existing nonzero structure of the output vector is cleared whenever the dense descriptor is not given. It fixes the corresponding unit test, and replaces the `grb::set( vector, mask, vector )` used by the label propagation algorithm (used for clamping) with the in-place `grb::foldl( vector, mask, vector )` using the `right_assign` operator. This MR also includes the related changes and fixes listed below. - Bugfix: introduces the previously missing masked vector-to-vector `grb::foldl` in the BSP1D backend; - Bugfix: masked vector-to-vector folds with dense vectors but sparse masks would return `ILLEGAL` (the generic implementation could execute it, but did not because it was executing a dynamic check on the dense descriptor using a template argument that was not the exact reverse of being passed that descriptor-- this MR fixes this by lifting checks w.r.t. the dense descriptor out of the generic implementation); - Bugfix: several variants of `grb::set` did not correctly return `ILLEGAL` when the dense descriptor was used; - Performance: exchange `grb::set` with `std::swap` at various places in the label propagation algorithm; - Performance: `grb::set` variants now do not call `assignAll` nor `clear` when a dense structure is assured to remain; - Testing: unify output from label smoke tests and minor typo fix in `smoketests.sh`; - Testing: `grb::set` unit test now also tests for correct behaviour when the dense descriptor is used; - Code improvements: various code style fixes across files touched by this MR; - Semantics & documentation: dense descriptor also applies to output containers. Thanks to Aristeidis for finding the bug and implementing the initial fix to `grb::set`!
1 parent 9d78cd8 commit 539a994

File tree

9 files changed

+819
-172
lines changed

9 files changed

+819
-172
lines changed

‎include/graphblas/algorithms/label.hpp‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ namespace grb {
116116
* accelerating the PageRank computation', ACM Press, 2003.
117117
*/
118118
template< typename IOType >
119-
RC label( Vector< IOType > &out,
119+
RC label(
120+
Vector< IOType > &out,
120121
const Vector< IOType > &y, const Matrix< IOType > &W,
121122
const size_t n, const size_t l,
122123
const size_t MaxIterations = 1000
@@ -230,7 +231,12 @@ namespace grb {
230231
<< "nnz( mask ) = " << nnz( mask ) << "\n";
231232
#endif
232233
// clamps the first l labelled nodes
233-
ret = ret ? ret : set( fNext, mask, f );
234+
ret = ret ? ret : foldl(
235+
fNext, mask,
236+
f,
237+
grb::operators::right_assign< IOType >()
238+
);
239+
assert( ret == SUCCESS );
234240
#ifdef _DEBUG
235241
std::cerr << "\t post-set nnz( fNext ) = " << nnz( fNext ) << "\n";
236242
printVector(
@@ -246,31 +252,36 @@ namespace grb {
246252
#ifdef _DEBUG
247253
std::cerr << "\t pre-set nnz(f) = " << nnz( f ) << "\n";
248254
#endif
249-
ret = ret ? ret : set( f, fNext );
255+
std::swap( f, fNext );
250256
#ifdef _DEBUG
251257
std::cerr << "\t post-set nnz(f) = " << nnz( f ) << "\n";
252258
#endif
253259
// go to next iteration
254-
(void)++iter;
260+
(void) ++iter;
255261
}
256262

257263
if( ret == SUCCESS ) {
258264
if( different ) {
259265
if( s == 0 ) {
260-
std::cout << "Warning: label propagation did not converge after "
266+
std::cerr << "Info: label propagation did not converge after "
261267
<< (iter-1) << " iterations\n";
262268
}
263269
return FAILED;
264270
} else {
265271
if( s == 0 ) {
266-
std::cout << "Info: label propagation converged in "
272+
std::cerr << "Info: label propagation converged in "
267273
<< (iter-1) << " iterations\n";
268274
}
269-
return set( out, f );
275+
std::swap( out, f );
276+
return SUCCESS;
270277
}
271278
}
272279

273280
// done
281+
if( s == 0 ) {
282+
std::cerr << "Warning: label propagation exiting with " << toString(ret)
283+
<< "\n";
284+
}
274285
return ret;
275286
}
276287

‎include/graphblas/bsp1d/blas1.hpp‎

Lines changed: 175 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,178 @@ namespace grb {
657657
return ret;
658658
}
659659

660+
/** \internal No implementation notes */
661+
template<
662+
Descriptor descr = descriptors::no_operation, class OP,
663+
typename IOType, typename MaskType, typename InputType,
664+
typename Coords
665+
>
666+
RC foldl(
667+
Vector< IOType, BSP1D, Coords > &x,
668+
const Vector< MaskType, BSP1D, Coords > &m,
669+
const Vector< InputType, BSP1D, Coords > &y,
670+
const OP &op = OP(),
671+
const Phase &phase = EXECUTE,
672+
const typename std::enable_if< grb::is_operator< OP >::value &&
673+
!grb::is_object< IOType >::value &&
674+
!grb::is_object< MaskType >::value &&
675+
!grb::is_object< InputType >::value, void
676+
>::type * = nullptr
677+
) {
678+
// static sanity checks
679+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
680+
std::is_same< typename OP::D1, IOType >::value ),
681+
"grb::foldl",
682+
"called with a vector x of a type that does not match the first domain "
683+
"of the given operator" );
684+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
685+
std::is_same< typename OP::D2, InputType >::value ),
686+
"grb::foldl",
687+
"called on a vector y of a type that does not match the second domain "
688+
"of the given operator" );
689+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
690+
std::is_same< typename OP::D3, IOType >::value ),
691+
"grb::foldl",
692+
"called on a vector x of a type that does not match the third domain "
693+
"of the given operator" );
694+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
695+
std::is_same< bool, MaskType >::value ),
696+
"grb::foldl",
697+
"called with a mask that does not have boolean entries " );
698+
699+
// catch empty mask
700+
if( size( m ) == 0 ) {
701+
return foldl< descr >( x, y, op, phase );
702+
}
703+
704+
// dynamic sanity checks
705+
const size_t n = size( x );
706+
if( n != size( y ) || n != size( m ) ) {
707+
return MISMATCH;
708+
}
709+
710+
// handle trivial resize phase
711+
if( config::IMPLEMENTATION< BSP1D >::fixedVectorCapacities() &&
712+
phase == RESIZE
713+
) {
714+
return SUCCESS;
715+
}
716+
717+
// delegate
718+
RC ret = foldl< descr >(
719+
internal::getLocal( x ), internal::getLocal( m ),
720+
internal::getLocal( y ),
721+
op, phase
722+
);
723+
724+
if( !config::IMPLEMENTATION< BSP1D >::fixedVectorCapacities() ) {
725+
if( collectives< BSP1D >::allreduce(
726+
ret, grb::operators::any_or< RC >()
727+
) != SUCCESS ) {
728+
return PANIC;
729+
}
730+
}
731+
732+
// handle try and execute phases
733+
if( phase != RESIZE ) {
734+
if( ret == SUCCESS ) {
735+
ret = internal::updateNnz( x );
736+
} else if( ret == FAILED ) {
737+
const RC subrc = internal::updateNnz( x );
738+
if( subrc != SUCCESS ) { ret = PANIC; }
739+
}
740+
}
741+
742+
// done
743+
return ret;
744+
}
745+
746+
/** \internal No implementation notes */
747+
template<
748+
Descriptor descr = descriptors::no_operation, class Monoid,
749+
typename IOType, typename MaskType, typename InputType,
750+
typename Coords
751+
>
752+
RC foldl(
753+
Vector< IOType, BSP1D, Coords > &x,
754+
const Vector< MaskType, BSP1D, Coords > &m,
755+
const Vector< InputType, BSP1D, Coords > &y,
756+
const Monoid &monoid = Monoid(),
757+
const Phase &phase = EXECUTE,
758+
const typename std::enable_if< grb::is_monoid< Monoid >::value &&
759+
!grb::is_object< IOType >::value &&
760+
!grb::is_object< MaskType >::value &&
761+
!grb::is_object< InputType >::value, void
762+
>::type * = nullptr
763+
) {
764+
// static sanity checks
765+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
766+
std::is_same< typename Monoid::D1, IOType >::value ),
767+
"grb::foldl",
768+
"called with a vector x of a type that does not match the first domain "
769+
"of the given operator" );
770+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
771+
std::is_same< typename Monoid::D2, InputType >::value ),
772+
"grb::foldl",
773+
"called on a vector y of a type that does not match the second domain "
774+
"of the given operator" );
775+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
776+
std::is_same< typename Monoid::D3, IOType >::value ),
777+
"grb::foldl",
778+
"called on a vector x of a type that does not match the third domain "
779+
"of the given operator" );
780+
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
781+
std::is_same< bool, MaskType >::value ),
782+
"grb::foldl",
783+
"called with a mask that does not have boolean entries" );
784+
785+
// catch empty mask
786+
if( size( m ) == 0 ) {
787+
return foldl< descr >( x, y, monoid, phase );
788+
}
789+
790+
// dynamic sanity checks
791+
const size_t n = size( x );
792+
if( n != size( y ) || n != size( m ) ) {
793+
return MISMATCH;
794+
}
795+
796+
// handle trivial resize phase
797+
if( config::IMPLEMENTATION< BSP1D >::fixedVectorCapacities() &&
798+
phase == RESIZE
799+
) {
800+
return SUCCESS;
801+
}
802+
803+
// delegate
804+
RC ret = foldl< descr >(
805+
internal::getLocal( x ), internal::getLocal( m ),
806+
internal::getLocal( y ),
807+
monoid, phase
808+
);
809+
810+
if( !config::IMPLEMENTATION< BSP1D >::fixedVectorCapacities() ) {
811+
if( collectives< BSP1D >::allreduce(
812+
ret, grb::operators::any_or< RC >()
813+
) != SUCCESS ) {
814+
return PANIC;
815+
}
816+
}
817+
818+
// handle try and execute phases
819+
if( phase != RESIZE ) {
820+
if( ret == SUCCESS ) {
821+
ret = internal::updateNnz( x );
822+
} else if( ret == FAILED ) {
823+
const RC subrc = internal::updateNnz( x );
824+
if( subrc != SUCCESS ) { ret = PANIC; }
825+
}
826+
}
827+
828+
// done
829+
return ret;
830+
}
831+
660832
/** \internal No communication necessary, output is guaranteed dense. */
661833
template<
662834
Descriptor descr = descriptors::no_operation,
@@ -683,17 +855,17 @@ namespace grb {
683855

684856
// static checks
685857
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
686-
std::is_same< InputType1, typename Operator::D1 >::value ),
858+
std::is_same< InputType1, typename Operator::D1 >::value ),
687859
"grb::eWiseApply",
688860
"called with a left-hand input vector value type that does not match the "
689861
"first domain of the given operator " );
690862
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
691-
std::is_same< InputType2, typename Operator::D2 >::value ),
863+
std::is_same< InputType2, typename Operator::D2 >::value ),
692864
"grb::eWiseApply",
693865
"called with a right-hand input vector value type that does not match the second "
694866
"domain of the given operator" );
695867
NO_CAST_ASSERT( ( !(descr & descriptors::no_casting) ||
696-
std::is_same< OutputType, typename Operator::D3 >::value ),
868+
std::is_same< OutputType, typename Operator::D3 >::value ),
697869
"grb::eWiseApply",
698870
"called with an output value type that does not match the third domain of "
699871
"the given operator" );

‎include/graphblas/descriptors.hpp‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,11 +117,11 @@ namespace grb {
117117
static constexpr Descriptor structural_complement = structural | invert_mask;
118118

119119
/**
120-
* Indicates that all input vectors to an ALP/GraphBLAS primitive are
121-
* structurally dense.
120+
* Indicates that all input and output vectors to an ALP/GraphBLAS primitive
121+
* are structurally dense.
122122
*
123-
* If a user passes this descriptor but one or more vectors input to the call
124-
* are \em not structurally dense, then #ILLEGAL shall be returned.
123+
* If a user passes this descriptor but one or more vectors to the call are
124+
* \em not structurally dense, then #ILLEGAL shall be returned.
125125
*
126126
* \warning <em>All vectors</em> includes any vectors that operate as masks.
127127
* Thus if the primitive is to operate with structurally sparse masks
@@ -134,13 +134,18 @@ namespace grb {
134134
* passing this descriptor to such primitive indicates that also the
135135
* output vector is structurally dense.
136136
*
137+
* \warning For out-of-place operations with vector output(s), passing this
138+
* descriptor also demands that the output vectors are already
139+
* dense.
140+
*
137141
* \warning Vectors with explicit zeroes (under the semiring passed to the
138142
* related primitive) will be computed with explicitly.
139143
*
140144
* The benefits of using this descriptor whenever possible are two-fold:
141145
* 1) less run-time overhead as code handling sparsity is disabled;
142146
* 2) smaller binary sizes as code handling structurally sparse vectors is
143147
* not emitted (unless required elsewhere).
148+
*
144149
* The consistent use of this descriptor is hence strongly encouraged.
145150
*/
146151
static constexpr Descriptor dense = 16;

‎include/graphblas/reference/blas1.hpp‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,9 +1058,6 @@ namespace grb {
10581058
if( !sparse && nnz( to_fold ) < n ) {
10591059
return ILLEGAL;
10601060
}
1061-
if( masked && !sparse && nnz( *m ) < n ) {
1062-
return ILLEGAL;
1063-
}
10641061
if( phase == RESIZE ) {
10651062
return SUCCESS;
10661063
}
@@ -1852,6 +1849,12 @@ namespace grb {
18521849
return MISMATCH;
18531850
}
18541851

1852+
const size_t n = size( x );
1853+
1854+
if( descr & descriptors::dense ) {
1855+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
1856+
}
1857+
18551858
#ifdef _DEBUG
18561859
std::cout << "In foldr ([T]<-[T])\n";
18571860
#endif
@@ -1919,6 +1922,10 @@ namespace grb {
19191922
if( n != size( y ) || n != size( m ) ) {
19201923
return MISMATCH;
19211924
}
1925+
if( descr & descriptors::dense ) {
1926+
if( size( m ) > 0 && nnz( m ) != n ) { return ILLEGAL; }
1927+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
1928+
}
19221929

19231930
if( nnz( x ) < n || nnz( y ) < n ) {
19241931
return internal::fold_from_vector_to_vector_generic<
@@ -2036,6 +2043,9 @@ namespace grb {
20362043
if( n != size( y ) ) {
20372044
return MISMATCH;
20382045
}
2046+
if( descr & descriptors::dense ) {
2047+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
2048+
}
20392049

20402050
const Vector< bool, reference, Coords > * const null_mask = nullptr;
20412051
if( nnz( x ) < n || nnz( y ) < n ) {
@@ -2101,6 +2111,10 @@ namespace grb {
21012111
if( n != size( y ) || n != size( m ) ) {
21022112
return MISMATCH;
21032113
}
2114+
if( descr & descriptors::dense ) {
2115+
if( size( m ) > 0 && nnz( m ) != n ) { return ILLEGAL; }
2116+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
2117+
}
21042118

21052119
if( nnz( x ) < n || nnz( y ) < n ) {
21062120
return internal::fold_from_vector_to_vector_generic<
@@ -2580,6 +2594,9 @@ namespace grb {
25802594
if( n != size( y ) ) {
25812595
return MISMATCH;
25822596
}
2597+
if( descr & descriptors::dense ) {
2598+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
2599+
}
25832600

25842601
// all OK, execute
25852602
const Vector< bool, reference, Coords > * const null_mask = nullptr;
@@ -2703,6 +2720,9 @@ namespace grb {
27032720
if( n != size( y ) ) {
27042721
return MISMATCH;
27052722
}
2723+
if( descr & descriptors::dense ) {
2724+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
2725+
}
27062726

27072727
// all OK, execute
27082728
const Vector< bool, reference, Coords > * const null_mask = nullptr;
@@ -2771,6 +2791,10 @@ namespace grb {
27712791
if( n != size( y ) || n != size( m ) ) {
27722792
return MISMATCH;
27732793
}
2794+
if( descr & descriptors::dense ) {
2795+
if( size( m ) > 0 && nnz( m ) != n ) { return ILLEGAL; }
2796+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
2797+
}
27742798

27752799
// all OK, execute
27762800
if( nnz( x ) < n || nnz( y ) < n ) {
@@ -2838,6 +2862,10 @@ namespace grb {
28382862
if( n != size( y ) || n != size( m ) ) {
28392863
return MISMATCH;
28402864
}
2865+
if( descr & descriptors::dense ) {
2866+
if( size( m ) > 0 && nnz( m ) != n ) { return ILLEGAL; }
2867+
if( nnz( x ) != n || nnz( y ) != n ) { return ILLEGAL; }
2868+
}
28412869

28422870
// all OK, execute
28432871
if( nnz( x ) < n || nnz( y ) < n ) {

0 commit comments

Comments
 (0)