Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create compatibility between vectors and matrices #39

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

LucasChollet
Copy link

Vectors are now children of matrices, so they share the same set of properties.
This hierarchy change allows the deletions of some duplicate code that was
shared among the two classes.

Some algorithms have been rewritten to accept both vectors and matrices.

However, there is a regression, you now need to be explicit about both
dimensions of the matrix when using iota().

@LucasChollet LucasChollet marked this pull request as ready for review October 10, 2021 21:52
Vectors are now children of matrices, so they share the same set of properties.
This hierarchy change allows the deletions of some duplicate code that was
shared among the two classes.

Some algorithms have been rewritten to accept both vectors and matrices.
@calebzulawski
Copy link
Owner

calebzulawski commented Oct 25, 2021

Thanks for the PR! Sorry I took so long to get to this, I've been pretty busy lately. I like the direction this is going. @drubinstein had an idea though, why not go one step further with template <typename T, std::size_t... Dimension> struct Tensor? What do you think?

@LucasChollet
Copy link
Author

Thank you for this answer !
It could be neat, but is it possible to do it without dynamic allocation ?

@calebzulawski
Copy link
Owner

With the parameter pack the dimensions are still constants, so I think so.

@LucasChollet
Copy link
Author

LucasChollet commented Oct 25, 2021

I may be missing something, but I was wondering about the creation of the array.
The number of bracket pairs is determined by the number of parameters in the pack, and I don't know how to write it.

@calebzulawski
Copy link
Owner

Yeah, I'll have to think about that one more. I'm pretty sure it's possible, but not obvious to me either at the moment. I think it's probably possible with a recursive template.

@calebzulawski
Copy link
Owner

calebzulawski commented Oct 25, 2021

This works:

#include <cstdint>

template <typename T, std::size_t... Dimension>
struct data_impl;

template <typename T, std::size_t... Dimension>
using data_impl_t = typename data_impl<T, Dimension...>::type;

template <typename T>
struct data_impl<T> {
    using type = T;
};

template <typename T, std::size_t First, std::size_t... Dimension>
struct data_impl<T, First, Dimension...> {
    using type = data_impl_t<T, Dimension...>[First];
};

constexpr data_impl_t<int, 2, 3, 1> d {{{0}, {1}, {2}}, {{3}, {4}, {5}}}; // same as int[2][3][1]

@LucasChollet
Copy link
Author

Woww ! It's truly impressive !

As another way to tackle this problem, isn't it possible to flatten the array ?

With your example, the element 4 (in position (1, 2, 0)) in d will just be array[2 * 1 + 3 * 2 + 0 * 1]

If this solution work, wouldn't it be simpler ?

@calebzulawski
Copy link
Owner

That is possible, but the problem is that makes initialization frustrating--you need to also initialize it in one giant array. Using a multidimensional array is more ergonomic, I think.

@LucasChollet
Copy link
Author

Definitely agreeing for a simple initialization.
First I thought to add a constructor to allow it, but I just realize that we will need your trick to write the constructor 😅
I will try to work on this later this week

@drubinstein
Copy link
Contributor

I'm pretty sure most of the current operations can be ported over pretty easily if the new Tensor class supports:

  • generate
  • elementwise
  • combine row and column of matrix to be a slice.

Was talking with @calebzulawski and we're thinking about keeping the scala, vector, 2D matrix specializations for now but rewrite them with Tensor as the backing datastructure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants