tomstewart89 / BasicLinearAlgebra

A library for using matrices and linear algebra on Arduino
MIT License
185 stars 38 forks source link

Integer determinant calculation #80

Closed insalt-glitch closed 2 months ago

insalt-glitch commented 3 months ago

Resolves #73 . I added an overload for the determinant calculation with Bareiss algorithm. It is probably slightly slower than the LUDecomposition but is guaranteed to be correct. I hope this fine :).

tomstewart89 commented 3 months ago

Great work! Unfortunately if I compile this using the arduino IDE I get:

BasicLinearAlgebra/impl/NotSoBasicLinearAlgebra.h:327:15: error: 'enable_if' in namespace 'std' does not name a template type
 typename std::enable_if<std::is_integral<typename ParentType::DType>::value, typename ParentType::DType>::type
               ^~~~~~~~~

Offhand, there might be some way we can achieve the same effect with static_asserts? Failing that, if this implementation supports integer datatypes and works well then I'm happy to just drop the current implementation in favor of this one. Let me know how you go! Thanks for the PR!

insalt-glitch commented 3 months ago

Okay... two things:

Now, I changed the name of the two swap-functions that exist now, because they were not in line with the snake-case naming of the rest of the repository. Should we keep the original names for backwards compatibility?

Additionally, I'm unsure if we should keep the original swap implementation at all. It is basically a simplified version of std::swap in the standard library, and it is only used once in the entire repository. Again, your decision!

Let me know what you think :)

Incorrectone commented 3 months ago

I do not think the library type_traits is ported to some Arduino boards, and you will have to use dependencies for this.

  1. Forum post about type_traits: 1 2
  2. Possible dependency if you want to use them instead (Edit: I have not tried this yet, also it does not have the is_fundamental, it does however have is_same)
  3. Or one can always use their own implemented functions instead if you want no dependencies

I tried the pull library (idk how to say it) on Arduino Mega in Arduino IDE 2, and it shows that it cannot find the library.

In file included from ***\Arduino\libraries\BasicLinearAlgebra/BasicLinearAlgebra.h:181:0,
                 from ***:
***\Arduino\libraries\BasicLinearAlgebra/impl/NotSoBasicLinearAlgebra.h:3:10: fatal error: type_traits: No such file or directory
 #include <type_traits>
          ^~~~~~~~~~~~~
compilation terminated.
exit status 1

Compilation error: exit status 1
Incorrectone commented 3 months ago

So I do not know much about classes and coding practices, but can we not simply overload the Determinant function for taking some integer type?

typename ParentType::DType Determinant(const MatrixBase<ParentType, Dim, Dim, int> &A) without ever using type_traits?

As I tried this with insalt's integer Bareiss algorithm:

in NotSoBasicLinearAlgebra.h

// general case for floats and doubles
template <typename ParentType, int Dim>
typename ParentType::DType Determinant(const MatrixBase<ParentType, Dim, Dim, typename ParentType::DType> &A)
{
    Matrix<Dim, Dim, typename ParentType::DType> A_copy = A;

    auto decomp = LUDecompose(A_copy);

    typename ParentType::DType det = decomp.parity;

    for (int i = 0; i < Dim; ++i)
    {
        det *= decomp.U(i, i);
    }

    return det;
}

// Function specifically for ints

template <typename ParentType, int Dim>
typename ParentType::DType Determinant(const MatrixBase<ParentType, Dim, Dim, int> &A)
{}

Also one more thing for type conversion from float matrices to int and vice-versa

Can we add in ElementStorage.h

    template <typename CastType> 
    Matrix(const Matrix<Rows, Cols, CastType> &other){ 
        for (int i = 0; i < Rows; ++i)
        {
            for (int j = 0; j < Cols; ++j)
            {
                (*this)(i, j) = static_cast<DType>(other(i,j));
            }
        }
    }

This also gives the correct answer for integer matrices

insalt-glitch commented 3 months ago

I do not think the library type_traits is ported to some Arduino boards, and you will have to use dependencies for this.

1. Forum post about type_traits: [1](https://forum.arduino.cc/t/compilation-error-type-traits-library-not-found-when-using-umate-and-serial9b-libraries/1146309) [2](https://stackoverflow.com/questions/37575303/is-the-c-standard-library-fully-supported-on-arduino)

2. [Possible dependency if you want to use them instead](https://www.arduino.cc/reference/en/libraries/arxtypetraits/) (Edit: I have not tried this yet, also it does not have the is_fundamental, it does however have is_same)

3. Or one can always use their own implemented functions instead if you want no dependencies

I tried the pull library (idk how to say it) on Arduino Mega in Arduino IDE 2, and it shows that it cannot find the library.

In file included from ***\Arduino\libraries\BasicLinearAlgebra/BasicLinearAlgebra.h:181:0,
                 from ***:
***\Arduino\libraries\BasicLinearAlgebra/impl/NotSoBasicLinearAlgebra.h:3:10: fatal error: type_traits: No such file or directory
 #include <type_traits>
          ^~~~~~~~~~~~~
compilation terminated.
exit status 1

Compilation error: exit status 1

Thank you very much for investigating. I only tried with theArduino MKR Zero as a board and there it is apparently implemented. I will rewrite the implementation to remove the dependency. I also think, at least for me personally, a huge charm of this library is the fact that it does not have any other library dependencies. So I'll come up with a strategy on the weekend and present may new idea.

The template specialization for int does not sound very appealing to me, since it would only work for int32_t, what about int16_t, etc. I guess we should decide what to do about unsigned types though. There, the current implementation gives the wrong answer. Probably in these cases we should always return an int, or emit an error with static_assert? I'm open for suggestions.

insalt-glitch commented 2 months ago

Okay, I changed the implementation now such that the type-traits header is no longer included, and I hope everything should compile fine now. The question still remains whether we want the additional complexity that this changes brings, and if anyone has an idea to make it simpler.

The Bareiss Algorithm works on all types (both integers and floating-point), but it is quite a bit slower than the LU-Decomposition-based method. This might be a no-go for some number of people on an embedded system. Thus, I think we should keep that :).