Bugzilla – Bug 426
Behavior of sum() for Matrix<bool> is unexpected and confusing
Last modified: 2016-11-14 17:59:06 UTC
When using the .sum() method on a Matrix<bool> my code compiles and runs perfectly. However, it did not do what I'd expect, e.g.,
smaller.any(): 1 1 1 0 0 1
Coming from Matlab, I had expected sum() to add all the true values as integer (where true is 1):
What I had to use was count(),
Now, sum() works more like any()
- remove sum() for Matrix<bool> to avoid confusing (so it doesn't compile at all)
- forward sum() to count(), only for Matrix<bool>
*** Bug 637 has been marked as a duplicate of this bug. ***
At the very least, we should emphasize this behavior in the documentation of sum. If it's possible we could generate compile time warnings for sum<bool>() (or deprecate it or make it fail).
I'm against forwarding to count().
My preference would be to disable the sum() function for boolean matrices, as it is ambiguous (or at the least confusing) and we have alternative for the two possible interpretations: .any() and .count() . However, this would be an API change so it should probably wait till Eigen 4. In the mean time, I suggest we emit compile time warnings.
I fear that few people will read the documentation for sum(), so just documenting it may not be a solution.
Emitting compile-time warnings is likely not possible (we already tried hard to find such a trick). Disabling it might indeed break existing code. If we are positive, I'd rather say that would help fixing bad existing code! If we consider sum() of bools as a bug, maybe we can still disable it.
Created attachment 375 [details]
Suggestion how to deprecate sum<bool>()
The attached patch would generate deprecated warnings when sum() is called for boolean expressions _and_ additionally gives the result of count() instead of sum().
Unfortunately, I don't know a way to give more informative messages inside a deprecated attribute.
The proposed patch would also deprecate things such as:
Eigen::Array<bool, 2, 2> A, B
// warning: ‘Eigen::internal::scalar_sum_op<bool>::scalar_sum_op()’ is deprecated
A += B;
// But this is not implemented either:
A |= B;
// Only clean workaround possible at the moment:
A = A || B;
Actually, we should prohibit or deprecate using A-B, A*B and A/B as well.
And we might think about allowing boolean bitwise operators---I'd suggest even for floating point types, to allow things such as (but not limited to)
A &= ~ArrayXf::Zero(len); // equivalent to A = abs(A);
(In reply to comment #6)
> A &= ~ArrayXf::Zero(len); // equivalent to A = abs(A);
Should of course be:
(In reply to comment #5)
> Created attachment 375 [details]
> Suggestion how to deprecate sum<bool>()
I pushed this, including a tiny bit of documentation:
Bitwise boolean operators have been suggested in bug 272 and bug 405.
Actually, it not only deprecates A += B or A = A + B, but it triggers a compilation error since the scalar types do not match anymore. This change might thus break existing code. I'm not against it but, what is strange is that at the same time it introduces a new deprecated feature:
Array<int,2,2> D = A + B
Perhaps we should not work at the scalar_sum_op level, but deprecate individual features (operator+, sum, etc.)
Regarding bitwise operators on floats, this is already how pabs is implemented:
(In reply to comment #9)
> Actually, it not only deprecates A += B or A = A + B, but it triggers a
> compilation error since the scalar types do not match anymore. This change
These only give deprecated warnings here (gcc 4.7.1, with -Wall)
> might thus break existing code. I'm not against it but, what is strange is that
> at the same time it introduces a new deprecated feature:
> Array<int,2,2> D = A + B
For A and B of type bool, these don't compile:
../eigen3/Eigen/src/Core/Assign.h:493:3: error: ‘YOU_MIXED_DIFFERENT_NUMERIC_TYPES__YOU_NEED_TO_USE_THE_CAST_METHOD_OF_MATRIXBASE_TO_CAST_NUMERIC_TYPES_EXPLICITLY’ is not a member of ‘Eigen::internal::static_assertion<false>’
If they are int arrays, it compiles without warning at my machine.
> Perhaps we should not work at the scalar_sum_op level, but deprecate individual
> features (operator+, sum, etc.)
Yes, that sounds like a cleaner solution, indeed.
> Regarding bitwise operators on floats, this is already how pabs is implemented:
Yes, I was suggesting to make these operators publicly available.
oh, sorry this was due to a local change in scalar_sum_op.
I'm re-opening because in 3.3 we do have the following regression:
typedef Eigen::Array<bool,Dynamic,Dynamic> ArrayXXb;
ArrayXXb a(3,3), b(3,3);
ArrayXXb r = a+b;
that does not compile anymore (because a+b returns a int expression. See: http://stackoverflow.com/questions/40537884/eigen-3-3-sparsematrixbool-operations
On the other hand, the following now compiles with a deprecated warning:
ArrayXi r = a+b;
The new behavior is actually consistent with c++, so we should probably keep it, but we should at least propose an easy workaround by adding operators && and || to MatrixBase and SparseMatrixBase. Currently, they are only available in ArrayBase, and so inaccessible from SparseMatrix. As I don't see any other interpretations of such operators for the linear algebra matrix world, I don't see reason not to make them readily accessible. Please, correct me if I'm overseeing possible misusage.
Then we could also think about not marking ArrayXi r = a+b; as deprecated, because the dangerous usage (e.i., ArrayXXb r = a+b;) does not compile anymore!
Regarding +-*/: One could interprete + for booleans as XOR (and * as AND). That would be consistent with F_2 (or Z_2), i.e., adding and multiplying mod 2.
Therefore, I would generally prefer to forbid/deprecate + and *, to avoid confusion.
I have no objections against adding &&,|| (or &,|,^) operators.
I agree with the general ambiguity of bool + bool, but here, since mat<bool> + mat<bool> now returns a expr<int>, you cannot miss use such operators because implicit conversion from expr<int> to mat<bool> is forbidden.
Anyway, providing operator && ans the likes is much more important here.
Summary: Bug 426: move operator && and || to MatrixBase and SparseMatrixBase.
3.3 backport: https://bitbucket.org/eigen/eigen/commits/b795c0557249/