This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 100 - Finalize the array-as-scalar feature
Summary: Finalize the array-as-scalar feature
Status: DECISIONNEEDED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.0
Hardware: All All
: High Feature Request
Assignee: Gael Guennebaud
URL:
Whiteboard:
Keywords:
Depends on: ExprEval
Blocks: 3.x
  Show dependency treegraph
 
Reported: 2010-10-29 09:51 UTC by Gael Guennebaud
Modified: 2019-12-04 09:55 UTC (History)
5 users (show)



Attachments
Add Array*::operator=(Scalar), and explicit Array(scalar) (10.92 KB, patch)
2014-09-11 15:04 UTC, Gael Guennebaud
no flags Details | Diff

Description Gael Guennebaud 2010-10-29 09:51:33 UTC
This feature is far to be complete. In particular nesting arrays is incomplete.
Comment 1 Gael Guennebaud 2012-03-26 18:17:00 UTC
too late for 3.1 -> 3.2
Comment 2 Gael Guennebaud 2014-09-11 10:12:11 UTC
Before talking about nesting arrays, we are currently lacking automatic scalar to array conversions such as:

Array3f A;
float s;
A = s;
A = Array3f(s);

More precisely the following ctor and operator= are not implemented yet:

Array(Scalar s);
ArrayBase::operator=(Scalar s);

and I'm wondering whether there are good reasons for not having them. The operator= overload seems to be harmless to me and needed to be consistent with compound operators as:

A += s;

The ctor must also be declared explicit to avoid ambiguous conversions as in:

A += 1;

-> Shall '1' be converted to a float or to an Array3f prior to doing the addition?

Moreover, this ctor would be enabled for fixed size only, but we cannot avoid a possible confusion with the ctor for 1D dynamic sized arrays in generic code. In the worst case, the value of a fixed size array will be initialized with its own size instead of doing a nop. But since we are talking about fixed size object, the compiler should be able to optimize this useless initialization for us.

What do you think?
Comment 3 Christoph Hertzberg 2014-09-11 10:27:14 UTC
I agree that we should allow this, also that we should implement the constructor only for fixed-sized objects. 
I'm not sure about making the constructor explicit, I would consider it inconsistent if
  Array3f A(1.0f);  // Legal 
  Array3f B;B=1.0f; // Legal
  Array3f C = 1.0f; // Illegal

I don't see how in
  A += 1;
the compiler would consider converting 1 to Array3f, because operator+= will always be a template function, won't it? And there is no automatic type conversion for templated parameters, IIRC (because in most cases there is no way the compiler could guess the correct template parameter).
Comment 4 Gael Guennebaud 2014-09-11 10:35:24 UTC
operator+= is not templated on the scalar type:

Derived& operator+=(const Scalar& scalar);

Doing so would require metaprogramming magic to distinguish between valid scalar type, expressions, and invalid types. And we know how fragile these kind of tricks are.
Comment 5 Christoph Hertzberg 2014-09-11 10:45:23 UTC
(In reply to Gael Guennebaud from comment #4)
> operator+= is not templated on the scalar type:

Ok, I meant that there is no
  operator+=(const Array3f&),
but something like (I don't know the exact implementation, atm)
  template<class Derived> operator+=(const ArrayBase<Derived>&),

Automatic type conversion to const Scalar& is wanted and should be harmless here.
Automatic type conversion to const ArrayBase<Derived>& would be ambiguous (as you stated in comment 2) but it will not happen.
Comment 6 Gael Guennebaud 2014-09-11 11:15:05 UTC
Arf, you're right, there is no operator+=(const Array3f&). My bad.
Comment 7 Gael Guennebaud 2014-09-11 15:04:39 UTC
Created attachment 493 [details]
Add Array*::operator=(Scalar), and explicit Array(scalar)

Here is a patch for:

  Array22f a(2.2);
  a = 3.3;

However, I did not managed to implement a reliable implicit constructor from scalar mostly because of conflict with the existing generic ctor:
explicit template <typename T> Array(const T &);

Indeed, simply adding:

  Array(const Scalar &);

does not work if the argument is not exactly a Scalar, e.g.:

  Array2cf a = 1.1f;

which fails with:

  Array.h:164:25: note: candidate constructor not viable: no known conversion from 'float' to 'const Scalar &'
      (aka 'const std::__1::complex<float> &') for 1st argument

This is strange to me because the following is fine:

  std::complex<float> c = 1.1;

so I don't understand why it does not kwon conversion from float to complex<float>...

So I also tried to add a generic implicit template ctor properly guarded with an enable_if:

  template<typename T>
  Array(constT &value,
        typename internal::enable_if<Base::SizeAtCompileTime!=1
        && internal::is_convertible<T, Scalar>::value,T>::type* = 0);

but then the following fails to compile:

  Array22f a;
  a = 1;

because it can be interpreted as:

  a = float(1);

or

  a = Array22f(1);

So we would have to generalize operator= too with all the tricky and risky enable_if rules as we have for _init1. I don't want to go this way (recall haw many iterations we had to make _init1 not too broken, and it is not 100% sure that there is no shortcoming anymore).
Comment 8 Gael Guennebaud 2014-09-19 13:31:26 UTC
I applied the patch:

https://bitbucket.org/eigen/eigen/commits/e286bf806c30/
Changeset:   e286bf806c30
User:        ggael
Date:        2014-09-19 11:25:28+00:00
Summary:     Bug 100: add support for explicit scalar to Array conversion (as enable implicit conversion is much more tricky)

Not allowing implicit conversion might not be that bad after all, e.g., see:
http://comments.gmane.org/gmane.comp.lib.eigen/4839
Comment 9 Christoph Hertzberg 2014-09-22 13:40:45 UTC
API-wise, I would not see a big danger in allowing implicit conversion in this case. But I agree that if it is too tricky, it is not really worth the effort.
Comment 10 Gael Guennebaud 2016-01-28 12:55:18 UTC
Next step is improving support with nested arrays -> 3.4
Comment 11 Nobody 2019-12-04 09:55:53 UTC
-- 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/100.

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