Re: smalleset() ( Re: [eigen] Householder.h: ::min() and operator<=)

[ Thread Index | Date Index | More lists.tuxfamily.org/eigen Archives ]


On 2016-09-12 17:09, Pavel Holoborodko wrote:
Probably the easiest way would be to require CustomType to have
constructor(s) for int (and other standard types).
The full set of constructors is not too long, e.g. in mpreal:

    // Constructors && type conversions
    mpreal();
    mpreal(const mpreal& u);
    mpreal(const long double u,            ...);
    mpreal(const unsigned long long int u, ...);
    mpreal(const long long int u,          ...);
    mpreal(const unsigned long int u,      ...);
    mpreal(const unsigned int u,           ...);
    mpreal(const long int u,               ...);
    mpreal(const int u,                    ...);

These set makes life much easier.

Generally, I agree. Also it also makes sense to specialize operators mixing standard types and the custom type if it is cheaper than first converting to the custom type. We could just deprecate all our manual casting and deny fixing issues of ceres::jet users, until ceres fixes that (I think that was the main source of complaints about these). The only question is if we are willing to break existing code in order to make our code easier (and perhaps more efficient in several cases)?


Also I would vote for not introducing Eigen::complex.
Just because once created, custom std::complex can be plugged into any
library where std::complex is supported.

Yes, this would mainly just fix the Scalar(1) issues (which would be implicitly fixed, if we require implicit construction from int)


Christoph


On Mon, Sep 12, 2016 at 11:53 PM, Christoph Hertzberg <
chtz@xxxxxxxxxxxxxxxxxxxxxxxx> wrote:

On 2016-09-12 16:37, Pavel Holoborodko wrote:

I'd rather stick with std::numeric_limits::min():

I second that.


I'm fine with either way.
Of course we should then update our documentation on how to provide custom
types (i.e., explicitly say that one should specialize std::numeric_limits
and inherit NumTraits<...> from GenericNumTraits<...>).
Probably the historic reason for introducing the aliases into NumTraits
was to avoid the confusion with min() for integers vs min() for floating
point types)
A small advantage of having aliases in NumTraits would be that we could
  typedef NumTraits<Scalar> NT;
locally, in case we need multiple attributes of it.

It is good idea to specialize std::numeric_limits for custom numeric type.
Together with std::swap, which enables std algorithms to work with the
type efficiently.

And probably provide specialization of std::complex for the type.


Some time ago I actually tried to make unit tests for everything with a
minimal-example generic type (i.e., just encapsulating a float and
forwarding all operators and std-functions). One of the main obstacles was
that we often do things like
  Scalar(1) or Scalar(0)
which does not work, if Scalar=std::complex<CustomType> and CustomType
does not have an implicit constructor from int (and writing
Scalar(RealScalar(0)) everywhere would be annoying).
How should a user best specialize std::complex to avoid that (without
copying the whole std::complex implementation, and just adding another
constructor)? Unfortunately, one can't do
namespace std {
template<>
class complex<MyType> : public ::std::complex<MyType>
{/* add stuff here */};
}

Could/should we maybe make our own internal Eigen::complex type (which
inherits from std::complex, but adds the additional constructors)?
This would also make it easier to provide fixed abs() implementations for
compilers where it is broken (I briefly remember some issues with that).


Christoph




On Mon, Sep 12, 2016 at 11:23 PM, Gael Guennebaud
<gael.guennebaud@xxxxxxxxx> wrote:

I'd rather stick with std::numeric_limits::min():

std::numeric_limits has to be specialized for custom types anyways (it is
one of the rare cases where users are welcomed to enter in the std
namespace), and I don't see reason for exposing a different value for
std::numeric_limits::min() and NumTraits::smallest()? The only advantage
I
see could be a clearer name (not sure smallest is really clearer though).

I know that we already have some aliases in NumTraits, but
retrospectively I
think this was a bad idea, it just make everything more confusing.

gael

On Mon, Sep 12, 2016 at 11:08 AM, Peter <list@xxxxxxxxxxxxxxxxx> wrote:


Dear Christoph,


Am 12.09.2016 um 10:57 schrieb Christoph Hertzberg:

If nobody objects against that, I'll patch this into the dev-branch.
Especially, are there objections against or alternative suggestions
for the
naming? `lowest()` probably would be a bad choice, since this is what
std
calls what
we call `min()`.


The problem with std::min() is, that it has to be understood
historically.
The name is a bad choice,
since the behaviour is different for integer and floating point values .

N.B.: The 1 must be T(1) here (in case T is not implicitly constructible
from int). And perhaps we should document it as "lowest
non-denormalized
positive value".


Which would suggest lowest() as a name, which is however already used.

Best regards,
Peter







--
 Dipl. Inf., Dipl. Math. Christoph Hertzberg

 Universität Bremen
 FB 3 - Mathematik und Informatik
 AG Robotik
 Robert-Hooke-Straße 1
 28359 Bremen, Germany

 Zentrale: +49 421 178 45-6611

 Besuchsadresse der Nebengeschäftsstelle:
 Robert-Hooke-Straße 5
 28359 Bremen, Germany

 Tel.:    +49 421 178 45-4021
 Empfang: +49 421 178 45-6600
 Fax:     +49 421 178 45-4150
 E-Mail:  chtz@xxxxxxxxxxxxxxxxxxxxxxxx

 Weitere Informationen: http://www.informatik.uni-bremen.de/robotik





--
 Dipl. Inf., Dipl. Math. Christoph Hertzberg

 Universität Bremen
 FB 3 - Mathematik und Informatik
 AG Robotik
 Robert-Hooke-Straße 1
 28359 Bremen, Germany

 Zentrale: +49 421 178 45-6611

 Besuchsadresse der Nebengeschäftsstelle:
 Robert-Hooke-Straße 5
 28359 Bremen, Germany

 Tel.:    +49 421 178 45-4021
 Empfang: +49 421 178 45-6600
 Fax:     +49 421 178 45-4150
 E-Mail:  chtz@xxxxxxxxxxxxxxxxxxxxxxxx

 Weitere Informationen: http://www.informatik.uni-bremen.de/robotik



Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/