Re: [eigen] Two issues in Sparse/RandomSetter.h

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

Hi Matthew,

I'm surprised you are not using the Google's variants and trying to use the GNU hash_map ;)

Anyway, indeed there are some inconsistencies. Actually, the support for GNU hash_map (GnuHashMapTraits) has been removed in favor of the more standard but not yet completely standard std::unordered_map via StdUnorderedMapTraits. The main problem is that the doc and the defaults have not been updated.

So I'd recommend you to use std::unordered_map or one of the more efficient Google's variants. If you really have to use GNU hash_map, then I'd recommend you to define your own traits class outside Eigen.

For the preferred default order, there is indeed an inconsistency too. In my opinion this should be: Dense/Sparse/std::unordered_map/std::map.

I don't have time to fix these inconsistencies right now, so as always patch welcome :)

I'll fill a bug report anyway to not forget about it.

gael

On Wed, Apr 28, 2010 at 10:36 PM, Matthew Hancher <mdh@xxxxxxxxxx> wrote:

Hi Eigenpeople,

I'm having a couple of isues with Sparse/RandomSetter.h.  This is with eigen 2.0.12 and a pretty standard Ubuntu gcc 4.2.4 installation, FWIW, but the problems appear to exist in tip and not be particularly platform-dependent.  I will happily open bug(s) about these, but I'd love a quick sanity-check before I do so.

First, despite efforts to support the GNU hash_map extension, it actually fails to do so.  In particular, this innocent-looking sequence of includes:

  #include <ext/hash_map>
  #include <Eigen/Sparse>

does not compile, complaining:

  .../Sparse/RandomSetter.h:162: error: invalid default argument for a template paramter

The problem is that Sparse/RandomSetter.h never defines GnuHashMapTraits, like it does for the Google hash map variants, so when it tries to use it on line 162 bad things happen.  It seems to me that the solution is to define GnuHashMapTraits earlier in that file.  I can submit a patch to do this if you think I'm on the right track.

Second, that same template argument is documented (at line 146) as defaulting to GoogleSparseHashMapTraits if the Google sparse_hash_map is available, but in fact the implementation ignores that altogether and instead defaults to GoogleDenseHashMapTraits if that is avilable, which doesn't even appear at all in the documented default preference order.  It seems to me that the solution there is to pick one preference order (either Dense/Gnu/Std, Sparse/Gnu/Std, or Sparse/Dense/Gnu/Std) and adjust both the documentation comment and the implementation to use that order.  I'll happily submit a patch for that too, if someone who understands the implications of changing the default can tell me what the best order is.

Thanks,
Matt