1144
2016-01-07 10:37:58 +0000
Regression in 3.3_beta1
2019-12-04 15:19:27 +0000
1
1
1
Unclassified
Eigen
Core - expression templates
3.3 (current stable)
All
All
RESOLVED
FIXED
Highest
Wrong Result
---
558
1
gladky.anton
eigen.nobody
chtz
gael.guennebaud
jacob.benoit.1
oldest_to_newest
5294
0
gladky.anton
2016-01-07 10:37:58 +0000
3.3_beta1 has a regression, which is demonstrated by the following code:
Eigen::Vector3f x(5,5,5);
Eigen::Matrix3f m = x.asDiagonal();
x = Eigen::Vector3f::Zero()+m*x;
In this case x will be zero [0,0,0], but should be [25,25,25].
Changing the order components "fixes" the problem:
x = Eigen::Vector3f::Zero()+m*x;
5295
1
chtz
2016-01-07 11:15:33 +0000
I can confirm this. The same happens if Vector3f::Zero() is replaced by
Vector3f x(5,5,5), y(0,0,0);
Matrix3f m = x.asDiagonal();
x = y + m*x;
It seems that evaluator_traits::AssumeAliasing is false and the expression gets evaluated as:
x = y;
x += m*x;
5296
2
gladky.anton
2016-01-07 12:17:32 +0000
The "fixed" version in my case should be:
x = m*x + Eigen::Vector3f::Zero();
5297
3
gael.guennebaud
2016-01-07 16:59:50 +0000
thanks for the finding, I'm on it. I'll probably move this AssumeAliasing attribute to evaluator::Flags.
5302
4
gael.guennebaud
2016-01-09 08:00:09 +0000
Partly fixed in: https://bitbucket.org/eigen/eigen/commits/0db197c797c3/
however, there is still a regression when the destination is resized and that the Product expression is hidden, as:
x = (M*x).cwiseAbs();
In 3.2, M*x was evaluated at the construction of the cwiseAbs expression, and therefore x was used before we reached operator=. In 3.3, x is resized before M*x is evaluated within a temporary. If "assume-aliasing" is transferred to cwiseAbs, then we will end-up with two temps instead of one: one for M*x and one for the complete right-hand side because of the "assume-aliasing" flag.
The only solution I can think of, would be to create the right-hand side evaluator before we resize the destination. That's easy to say, but that's inconvenient to implement because this would require to duplicate the resize logic (which currently occur once) in all specializations of Assignment... and this is thus error prone too. And we would do this for only one particular case...
Overall, I would vote for not fixing that one. Finally, it is not that much different than:
x = x.segment(...);
for which we do not anything special...
5305
5
chtz
2016-01-11 13:48:25 +0000
We could whenever dest needs to be resized, evaluate the source into a temporary and then move/swap the result into the actual destination.
Unfortunately, this needs to be detected at run-time and likely requires an additional branch. Also, this won't work for matrices where Max(Rows|Cols)AtCompiletime is set.
If that's too inefficient or to complicated to implement, I'm ok with leaving aliased expressions which require resizing as undefined.
5308
6
gael.guennebaud
2016-01-12 08:44:55 +0000
and that double the number of temporaries, for instance in:
x = (M*x).cwiseAbs();
with M non-square, we would do:
temp1 = M*x;
temp2 = temp1.cwiseAbs();
x = temp2;
Actually, that's what I first did when trying to fix the original regression.
5309
7
chtz
2016-01-12 09:16:44 +0000
If you wrote
MatrixXd A,B,temp;
temp.noAlias() = (A*B).cwiseAbs();
this should be equivalent to
temp.noAlias() = A*B; temp = temp.cwiseAbs();
right?
So (in case swap is O(1)) we could translate this
B = (A*B).cwiseAbs();
into
temp.noAlias() = (A*B).cwiseAbs();
B.swap(temp);
I'm not saying it's trivial to implement, though ...
5310
8
chtz
2016-01-12 21:21:22 +0000
(In reply to Christoph Hertzberg from comment #7)
> temp.noAlias() = (A*B).cwiseAbs();
> this should be equivalent to
> temp.noAlias() = A*B; temp = temp.cwiseAbs();
I did some debugging and now see that my assumption was wrong. I admit that this is difficult, for example since it won't work for things like the following:
C.noalias() += (A*B).cwiseAbs();
I don't see a good solution for avoiding a temporary here (using lazyProduct would avoid the temporary, of course).
I guess if it only resizing assignments cause trouble now, it is not really worth blocking 3.3 for that -- putting a warning about that in the release notes would be mandatory then, of course.
5349
9
gael.guennebaud
2016-01-25 15:14:17 +0000
Clarified doc:
https://bitbucket.org/eigen/eigen/commits/4bb9a076d0da/
Summary: Bug 1144: clarify the doc about aliasing in case of resizing and matrix product.
and 3.3 summary page:
http://eigen.tuxfamily.org/index.php?title=3.3#Changes_that_might_impact_existing_code
9588
10
eigen.nobody
2019-12-04 15:19:27 +0000
-- GitLab Migration Automatic Message --
This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.
You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1144.