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

Replace Gram-Schmidt orthogonalisation with better algorithm #679

Closed
mscroggs opened this issue Aug 4, 2023 · 4 comments · Fixed by #785
Closed

Replace Gram-Schmidt orthogonalisation with better algorithm #679

mscroggs opened this issue Aug 4, 2023 · 4 comments · Fixed by #785
Labels
enhancement New feature or request
Milestone

Comments

@mscroggs
Copy link
Member

mscroggs commented Aug 4, 2023

When elements are created, the polynomial basis is orthogonalised:

template <std::floating_point T>
void orthogonalise(mdspan_t<T, 2>& wcoeffs)
{
for (std::size_t i = 0; i < wcoeffs.extent(0); ++i)
{
for (std::size_t j = 0; j < i; ++j)
{
T a = 0;
for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
a += wcoeffs(i, k) * wcoeffs(j, k);
for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
wcoeffs(i, k) -= a * wcoeffs(j, k);
}
T norm = 0;
for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
norm += wcoeffs(i, k) * wcoeffs(i, k);
for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
wcoeffs(i, k) /= std::sqrt(norm);
}
}

This orthogonalisation currently uses the Gram-Schmidt process. Instead, we should use a better algorithm (eg. Computing Q from the QR decomposition of A^T)

@mscroggs mscroggs added the enhancement New feature or request label Aug 4, 2023
@ampdes
Copy link
Contributor

ampdes commented Feb 4, 2024

The above function is now in math.h.

The Gram-Schmidt process constructs the Q matrix in-place whereas the R matrix can be constructed by collecting the a s (the a terms in above code snippet).

One alternative is the Householder reflection algorithm which constructs the R matrix in-place which is not sought here.

However, the current implementation is Classical Gram-Schmidt
https://github.com/FEniCS/basix/blob/44962386b6a5f06f64ec956cdd5737984df81f32/cpp/basix/math.h#L368C1-L391C1

can easily be changed to the numerically stable modified Gram-Schmidt:

  for (std::size_t i = start; i < wcoeffs.extent(0); ++i)
  {
    T norm = 0;
    for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
      norm += wcoeffs(i, k) * wcoeffs(i, k);
    if (std::abs(norm) < 4 * std::numeric_limits<T>::epsilon())
    {
      throw std::runtime_error(
          "Cannot orthogonalise the rows of a matrix with incomplete row rank");
    }

    for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
      wcoeffs(i, k) /= std::sqrt(norm);

    for (std::size_t j = i+1; j < wcoeffs.extent(0); ++j)
    {
      T a = 0;
      for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
        a += wcoeffs(i, k) * wcoeffs(j, k);
      for (std::size_t k = 0; k < wcoeffs.extent(1); ++k)
        wcoeffs(j, k) -= a * wcoeffs(i, k);
    }

  }

@mscroggs
Copy link
Member Author

mscroggs commented Feb 5, 2024

This orthogonalisation is now used in fewer places than it once was, so modified Gram-Schmidt should be good enough. Do you want to open a pull request to make the modification you suggested?

@ampdes
Copy link
Contributor

ampdes commented Feb 5, 2024

This only affects the custom element creation?
For a PR, should this change be tested for test/test_custom_element.py?

@mscroggs mscroggs added this to the v0.8.0 milestone Feb 6, 2024
ampdes added a commit to ampdes/basix that referenced this issue Feb 9, 2024
…S#679)

* change the orthogonalization algorithm to modified gram schmidt

* norm is precomputed

* first check row rank before orthogonalizing the rows
ampdes added a commit to ampdes/basix that referenced this issue Feb 9, 2024
…S#679)

* change the orthogonalization algorithm to modified gram schmidt

* norm is precomputed

* first check row rank before orthogonalizing the rows
ampdes added a commit to ampdes/basix that referenced this issue Feb 9, 2024
…S#679)

* change the orthogonalization algorithm to modified gram schmidt

* norm is precomputed

* first check row rank before orthogonalizing the rows
@ampdes
Copy link
Contributor

ampdes commented Feb 9, 2024

The existing implementation is neither the classical Gram Schmidt or the more commonly known modified Gram Schmidt.
A classical Gram Schmidt would use an auxiliary vector to store the orthogonalization result before scaling.
This version (existing implementation) appears in https://people.inf.ethz.ch/gander/papers/qrneu.pdf as a variant of modified Gram Schmidt.

None the less the commonly known modified Gram Schmidt is added by #785

github-merge-queue bot pushed a commit that referenced this issue Feb 9, 2024
…#785)

* change the orthogonalization algorithm to modified gram schmidt

* norm is precomputed

* first check row rank before orthogonalizing the rows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants