This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 1531 - Add compile-time access to Eigen::Map dimensionality
Summary: Add compile-time access to Eigen::Map dimensionality
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: unspecified
Hardware: All All
: Normal Unknown
Assignee: Kasper Marstal
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-27 07:13 UTC by Kasper Marstal
Modified: 2019-12-04 17:34 UTC (History)
4 users (show)



Attachments
Adds NumDimensions to Eigen/src/Core/DenseBase.h (887 bytes, patch)
2018-04-11 17:44 UTC, Kasper Marstal
kaspermarstal: review+
Details | Diff
Add NumDimensions to Matrix, MatrixMap, Array and ArrayMap and tests thereof (7.77 KB, patch)
2018-04-25 10:12 UTC, Kasper Marstal
no flags Details | Diff
Adds NumDimensions to ArrayBase (432 bytes, patch)
2018-04-25 10:20 UTC, Kasper Marstal
no flags Details | Diff
Add NumDimensions to mapped matrices and mapped arrays (5.77 KB, patch)
2018-04-25 15:12 UTC, Kasper Marstal
no flags Details | Diff
Add NumDimensions to SolverBase and SparseMatrixBase (1.71 KB, patch)
2018-04-25 18:41 UTC, Kasper Marstal
no flags Details | Diff

Description Kasper Marstal 2018-03-27 07:13:48 UTC
We would like to propose compile-time access to Eigen::Map dimensionality.

To align the Map API as must as possible with the TensorMap API, NumIndices and NumDimensions could be exposed on the Map class and perhaps also on the Matrix class itself.

Our use-case is a medical image processing library where compile-time access to image dimensionality is needed. As the medical imaging world is moving to Eigen, e.g. ITK and VTK, I can imagine this will be useful for other developers in future.

I am happy to contribute a patch with whatever the maintainers deem the best name (NumDimensions or NumIndices or something else), the most appropriate implementation, and the best place to put it (e.g. in matrix class or only in map class). 

Thoughts? Comments?
Comment 1 Kasper Marstal 2018-03-27 07:22:23 UTC
Clarification: While there is compile-time access to dimensionality via IsVectorAtCompileTime, it would be useful to have this as a number named NumIndices and/or NumIndices, so that Map and TensorMap classes can be used interchangeably.
Comment 2 Christoph Hertzberg 2018-03-27 17:23:27 UTC
If we add this, this should be added to DenseBase (where SizeAtCompileTime, et al. are defined).

Questions: What do we expect NumDimensions to be for
  1 x 1: 0, 1 or 2?
  1 x N: 1 or 2?
  N x 1: 1 or 2?
Comment 3 Kasper Marstal 2018-03-28 05:20:04 UTC
For maximum compatibility between the Map API and the TensorMap API, NumDimensions for 1 x 1 should be 0, NumDimensions for 1 x N should be 1, and NumDimensions for N x 1 should also be 1.

I hope this does not clash with definitions/terminology elsewhere in the library.
Comment 4 Kasper Marstal 2018-04-11 17:44:26 UTC
Created attachment 849 [details]
Adds NumDimensions to Eigen/src/Core/DenseBase.h

This patch adds NumDimensions to all dense objects (matrix, vector, arrays, and related expression types). The value is equal to NumDimensions for tensors with 0, 1, and 2 dimensions for Scalars, vectors, and matrices, respectively.
Comment 5 Kasper Marstal 2018-04-25 10:12:44 UTC
Created attachment 854 [details]
Add NumDimensions to Matrix, MatrixMap, Array and ArrayMap and tests thereof

This patch adds support for NumDimensions to Matrix, MatrixMap, Array and ArrayMap. A helper function was added to Eigen/src/Core/util/XprHelper.h, which is used to set NumDimensions in DenseBase, which in turn is added EIGEN_GENERIC_PUBLIC_INTERFACE. I hope this is the eigen-way to add this functionality. Let me know if you have suggestions for changes/style.

There are two limitations of this patch: 
* I could not figure out how to make new CALL_SUBTEST_# commands, so tests are added to an existing subtest number. If you would tell me how, and I am happy to add tests to a separate subtest number.
* This is a git diff. I tested `hg import num_dimensions.patch` and it worked fine, but let me know if you run into any problems.
Comment 6 Kasper Marstal 2018-04-25 10:20:37 UTC
Created attachment 855 [details]
Adds NumDimensions to ArrayBase

This patch Adds NumDimensions to ArrayBase (array tests in patch 854 compiled fine without it, but this patch may be added for completeness).
Comment 7 Christoph Hertzberg 2018-04-25 11:49:59 UTC
Comment on attachment 854 [details]
Add NumDimensions to Matrix, MatrixMap, Array and ArrayMap and tests thereof

The following:
      NumDimensions = (internal::num_dimensions<internal::traits<Derived>::MaxRowsAtCompileTime, internal::traits<Derived>::MaxColsAtCompileTime>::ret),

could be just:
 NumDimensions = int(MaxSizeAtCompileTime)==1 ? 0 : bool(IsVectorAtCompileTime) ? 1 : 2;

And unless `NumDimensions` is used within the `Matrix`, `Array`, ... classes, there is no need for `using Base::NumDimensions;`
Comment 8 Kasper Marstal 2018-04-25 15:12:01 UTC
Created attachment 856 [details]
Add NumDimensions to mapped matrices and mapped arrays

Thank you for your suggestions Christoph.

Non-mapped Matrix/Array NumDimensions is not needed for my particular use case, and perhaps it is better to leave it out (until someone needs it at least, in which case it is trivial to add). 

I have attached a patch with your suggestions.
Comment 9 Kasper Marstal 2018-04-25 18:41:41 UTC
Created attachment 857 [details]
Add NumDimensions to SolverBase and SparseMatrixBase

This patch adds NumDimensions to SolverBase and SparseMatrixBase. This is necessary because the previous patch added NumDimensions to EIGEN_GENERIC_PUBLIC_INTERFACE. 

If you are not happy with this, and you can think of another solution that does not require changing EIGEN_GENERIC_PUBLIC_INTERFACE, I am happy to implement it.
Comment 10 Kasper Marstal 2018-05-01 08:41:25 UTC
Dear all, shall we make a decision on this proposal?
Comment 11 Kasper Marstal 2018-05-11 10:45:37 UTC
Is there anything I can do help expedite this proposal?
Comment 13 Kasper Marstal 2018-06-22 11:00:40 UTC
Great! Thank you for your effort!
Comment 14 Nobody 2019-12-04 17:34:09 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/1531.

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