New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 426 - Behavior of sum() for Matrix<bool> is unexpected and confusing
Behavior of sum() for Matrix<bool> is unexpected and confusing
Status: REOPENED
Product: Eigen
Classification: Unclassified
Component: Core - general
3.0
All All
: High Unknown
Assigned To: Nobody
:
: 637 (view as bug list)
Depends on:
Blocks: 3.4
  Show dependency treegraph
 
Reported: 2012-03-02 12:44 UTC by iv0co
Modified: 2016-11-14 17:59 UTC (History)
5 users (show)



Attachments
Suggestion how to deprecate sum<bool>() (1.02 KB, text/plain)
2013-08-13 15:01 UTC, Christoph Hertzberg
no flags Details

Description iv0co 2012-03-02 12:44:43 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
smaller.any().sum(): 1

Coming from Matlab, I had expected sum() to add all the true values as integer (where true is 1):
smaller.any().sum(): 4

What I had to use was count(),
smaller.any().count(): 4

Now, sum() works more like any()

Proposed solutions:
- remove sum() for Matrix<bool> to avoid confusing (so it doesn't compile at all)
- forward sum() to count(), only for Matrix<bool>
Comment 1 Christoph Hertzberg 2013-08-01 08:41:23 UTC
*** Bug 637 has been marked as a duplicate of this bug. ***
Comment 2 Christoph Hertzberg 2013-08-01 08:46:39 UTC
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().
Comment 3 Jitse Niesen 2013-08-01 10:18:45 UTC
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.
Comment 4 Gael Guennebaud 2013-08-01 12:00:53 UTC
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.
Comment 5 Christoph Hertzberg 2013-08-13 15:01:17 UTC
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.
Comment 6 Christoph Hertzberg 2013-08-16 14:32:01 UTC
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)
  ArrayXf A(len);
  A &= ~ArrayXf::Zero(len); // equivalent to A = abs(A);
Comment 7 Christoph Hertzberg 2013-08-16 14:37:33 UTC
(In reply to comment #6)
>   A &= ~ArrayXf::Zero(len); // equivalent to A = abs(A);

Should of course be:
  A&= ~(-ArrayXf::Zero(len));
Comment 8 Christoph Hertzberg 2013-08-19 12:24:51 UTC
(In reply to comment #5)
> Created attachment 375 [details]
> Suggestion how to deprecate sum<bool>()

I pushed this, including a tiny bit of documentation:

https://bitbucket.org/eigen/eigen/commits/ea0dcdc16c8af840128f91e9b3132efad3ef7d02
https://bitbucket.org/eigen/eigen/commits/c88d409486842002835e88b2d1daaf43b1b20dbf

Bitwise boolean operators have been suggested in bug 272 and bug 405.
Comment 9 Gael Guennebaud 2013-08-19 15:00:06 UTC
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:

https://bitbucket.org/eigen/eigen/src/d6f1fb64c88dc9c37d5cf24ba8528678d0e157b4/Eigen/src/Core/arch/SSE/PacketMath.h?at=default#cl-362
Comment 10 Christoph Hertzberg 2013-08-19 16:14:38 UTC
(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.
Comment 11 Gael Guennebaud 2013-08-20 11:24:00 UTC
oh, sorry this was due to a local change in scalar_sum_op.
Comment 12 Gael Guennebaud 2016-11-12 21:07:27 UTC
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!
Comment 13 Christoph Hertzberg 2016-11-12 21:58:04 UTC
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.
Comment 14 Gael Guennebaud 2016-11-14 12:26:56 UTC
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.
Comment 15 Gael Guennebaud 2016-11-14 17:59:06 UTC
https://bitbucket.org/eigen/eigen/commits/6cc1f2a41be5/
Summary:     Bug 426: move operator && and || to MatrixBase and SparseMatrixBase.

3.3 backport: https://bitbucket.org/eigen/eigen/commits/b795c0557249/

Note You need to log in before you can comment on or make changes to this bug.