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

Improve error message in DenseMatrix::Eigensystem #1678

Closed
benzwick opened this issue Aug 3, 2020 · 5 comments
Closed

Improve error message in DenseMatrix::Eigensystem #1678

benzwick opened this issue Aug 3, 2020 · 5 comments

Comments

@benzwick
Copy link
Member

benzwick commented Aug 3, 2020

Attempting to compute the eigensystem of a dense matrix without LAPACK results in the following error:

DenseMatrix::Eigensystem
Aborted

which is not very informative.

Could we add some extra information to this line in densemat.cpp:

   mfem_error("DenseMatrix::Eigensystem");

e.g. like this:

   mfem_error("DenseMatrix::Eigensystem: requires MFEM built with LAPACK");

to make it immediately obvious why this failed?

Maybe the instructions for using LAPACK could also be added to the instructions on the website.

@benzwick
Copy link
Member Author

benzwick commented Aug 4, 2020

On line 1167 there is also this one:

   mfem_error("DenseMatrix::Eigensystem for generalized eigenvalues");

On line 3372 (and a few other places) there is this message which could be used for the similar errors above:

   mfem_error("DenseMatrixEigensystem::Eval(): Compiled without LAPACK");

There are also some other errors in densemat.cpp that could output more information like "incorrect matrix dimensions" or "dimension mismatch", for example here:

   if (A.Height() != ABt.Height() || B.Height() != ABt.Width() ||
       A.Width() != B.Width())
   {
      mfem_error("MultABt(...)");
   }

@barker29
Copy link
Member

barker29 commented Aug 4, 2020

In my view these suggestions are very reasonable. Could you collect them in a pull request and submit it?

@barker29 barker29 self-assigned this Aug 4, 2020
@benzwick
Copy link
Member Author

benzwick commented Aug 5, 2020

Yes, I will create a PR for this.

Somewhat related, can someone please explain to me what the MFEM_CONTRACT_VAR macro is used for. It is defined here as:

#define MFEM_CONTRACT_VAR(x) if (false && (&x)+1){}

and used a lot in densemat.cpp.

@barker29
Copy link
Member

barker29 commented Aug 5, 2020

If I understand correctly, MFEM_CONTRACT_VAR is a hack to suppress an unused variable warning in optimized build. If you use a variable in asserts but nothing else, the preprocessor will strip out all the uses and you get the warning, but if you use MFEM_CONTRACT_VAR then the optimizer will still remove the statement (it doesn't do anything) but not trigger the warning.

@tzanio
Copy link
Member

tzanio commented Aug 16, 2020

Discussion continued in #1685.

@tzanio tzanio closed this as completed Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants