[Meta] Eradicate exceptions without argument #610

Open
bangerth opened this Issue Feb 27, 2015 · 13 comments

Projects

None yet

4 participants

@bangerth
Member

One of the first lessons I teach my students is to read the error messages. Once they get used to it, it actually works reasonably well. But only for those errors that do actually print anything useful. Many of our messages do that, but we have ~250 exceptions declared with DeclException0 that, in essence, only print the name of the exception but nothing else. I think the original idea was that the name of the exception is a giveaway, but I find now that that was a failed hope.

In other words, we need to get rid of this!

@tjhei
Member
tjhei commented Feb 27, 2015

Good idea. I vote for adding a text argument (optional?) to ExcInternalError and ExcNotImplemented. The reason is that I often end up using ExcMessage because I can not add a short description. I guess ExcMessage should be a last resort.

I am happy to do this if you agree.

@bangerth
Member

I think ExcInternalError is truly an internal error only developers should be get (or that at the very least require a developer to fix). ExcNotImplemented could take an argument though I'm not sure what one would say? I'm definitely ok with it, though, if you want to add an argument and go through our code base to change it throughout (for both of these exceptions).

I've been using ExcMessage in the more recent past for all of those cases where there is no actual data to be printed (so where I would have used something declared with DeclException0 in the past). I think it's not just a last resort -- I think it's a legitimate way to provide some description of what the error means and how it may have happened. Whether you have a name for the exception class or not doesn't matter in that case.

@tjhei
Member
tjhei commented Feb 27, 2015

The reason I bring this up is because you introduced code of the form ExcMessage("Internal Error: ...") (see 96e0ffb). I don't care strongly about all this, but I always tried to shy away from ExcMessage because it is not descriptive, hard to grep for, and awful to catch. It is easy to have a sentence of why I think this is an internal error (I don't want to require a message of course).

@bangerth
Member

That's a good point. I'll have to think about this.

Maybe we should just think about an alternative way of using DeclException0. Let's say something like DeclExceptionMsg that creates an exception class that takes no argument, but where the exception declaration still contains a descriptive string. Thinking about it, that actually seems like the best of all worlds since it only requires changing the one place where we currently declare the exception. What do you think?

@tjhei
Member
tjhei commented Feb 28, 2015

Yes, that is what I meant. As long as we make it optional so we don't have to go through the whole library. Maybe something like (edit: or call it DeclExceptionMsg):

 #define DeclException0(Exception0, defaulttext) \
class Exception0 :  public ::ExceptionBase \
{ \
public: \
Exception0 (const std::string & msg = defaulttext) : arg1 (msg) {}                            \
virtual ~Exception0 () throw () {}                                    \
virtual void print_info (std::ostream &out) const {                   \
  out << arg1 << std::endl;                                       \
}                                                                     \
private:                                                                \
const std::string arg1;                                                     \
}

So we can do this:

   DeclException0 (ExcInternalError, "This exception usually indicates that some condition which the programmer thinks must be satisfied at a certain point in an algorithm, is not fulfilled.");

  AssertThrow(dim<3, ExcInternalError());
  Assert (next_unused_vertex < triangulation.vertices.size(),
                    ExcInternalError("Internal error: During refinement, the triangulation wants to access an element of the 'vertices' array but it turns out that the array is not large enough."));
@bangerth bangerth added a commit to bangerth/dealii that referenced this issue Mar 5, 2015
@bangerth bangerth Significantly improve an error message.
Addresses #610.
b7d69aa
@bangerth bangerth added a commit to bangerth/dealii that referenced this issue Mar 12, 2015
@bangerth bangerth Improve what an exception says.
This happens to be a case one of my students ran into today. Again for #610.
aecc7be
@simonsticko simonsticko added a commit to simonsticko/dealii that referenced this issue Mar 28, 2015
@bangerth @simonsticko bangerth + simonsticko Significantly improve an error message.
Addresses #610.
c1e57de
@simonsticko simonsticko added a commit to simonsticko/dealii that referenced this issue Mar 28, 2015
@bangerth @simonsticko bangerth + simonsticko Improve what an exception says.
This happens to be a case one of my students ran into today. Again for #610.
106647b
@guidokanschat
Contributor

There is also the option of documenting the exception classes... take that back, somebody killed that in doxygen.

Now, my problem with a lot of the current "improved" error messages is that I have to read through a page of prose before they get to the point. Which is something that does not encourage people to read them. And I feel myself getting tired after the first 3 lines and I cannot help teaching others to get tired as well.

As always: error messages cannot replace reading a book. If you do not know what convergence is, google the terms in the error message and read up on them.

@bangerth
Member
bangerth commented Apr 2, 2015

I admit that I think that it is your loss if you feel that you have to teach discontent with longer error messages.

I fully understand that you and I don't need to read long error messages. We've been doing this for 20 years -- we look at the location and the backtrace of the error and I know what I need to fix and where. But you will have made the experience that none of those in your deal.II classes will know what to do if they get an error message that simply says

  Additional Information:
    ExcNoConvergence

They will have to ask someone, or more often they spend a couple of days poking at more or less random places. Why not help them and point them to common causes? Sure, there will be cases where this does not help either, but I cannot see how you believe that longer error messages are generally a bad idea, or how documenting errors better in the places where they are encountered first would be a bad idea. It's not like you're going to see the error message every time you call your program (or if you do, you're doing something wrong :-).

@tamiko
Member
tamiko commented Apr 2, 2015

There is a difference between verbose information about an exception and a verbose explanation about the cause and reasons for an exception. Personally, I do not think that it helps very much to print several paragraphs of documentation in "Additional Information".

This is first of all a clear distraction for advanced users. Further, I doubt that it fulfills its primary purpose. From a psychological point of view, verbose error message do not get read - or not read thoroughfully - because an error message is, by its very nature, a surprising and demanding event connected with stress for inexperienced users.

Keeping in mind that this is my personal opinion on this matter, I suggest as a compromise to

  • Use an #ifdef doxygen guard to get doxygen coverage of the exception classes
  • Provide the full explanation in the doxygen documentation of the respective class.
  • Reduce the information in "Additional Information" to one/two descriptive sentences
  • Add a link to the full documentation in the "Additional Information" section that points to the full documentation. "More information can be found at: ..."

I think this is a reasonable compromise that

  • provides enough information to understand the error
  • still preserves the full documentation if needed
  • Does not distract the output of the abort message too much.
@tjhei
Member
tjhei commented Apr 2, 2015

Add a link to the full documentation in the "Additional Information" section that points to the full documentation. "More information can be found at: ..."

Do you mean we should print the URL that links to the doxygen page? That is a good idea if we can figure out how to automate this!

@tamiko
Member
tamiko commented Apr 2, 2015

Do you mean we should print the URL that links to the doxygen page?

Yes.

That is a good idea if we can figure out how to automate this!

Let's have a look at the doxygen documentation. I'm sure such a
functionality exists.

@tjhei
Member
tjhei commented Apr 2, 2015

Let's have a look at the doxygen documentation. I'm sure such a functionality exists.

One thing I found is http://www.stack.nl/~dimitri/doxygen/manual/external.html
We already generate the deal.tag file that we could parse to find the URL+anchor. It looks like the anchor is an md5 of the function signature, so we might just hard-code the URL and hope it doesn't change over night. :-)
Alternatively, we create a small script that forwards to the correct page by looking it up in deal.tag. The advantage is that we can make the URLs very short: http://dealii.org/explain?m=ExcInternalError

@bangerth
Member

I've given this some thought, and I've also taken a poll among my students. I believe that your arguments above start from the wrong place, though, namely:
1/ That everyone is like us in that they don't need explanatory error messages.
2/ That explanatory and possibly lengthy error messages are a general problem (as opposed to specific examples of long messages -- which I'm quite happy to discuss if you point them out).

Having observed students struggle with brief error messages and what to do about them, my assumption is that the more help we can provide them, the better. I may of course be wrong in my assumption, but I think I have empirical evidence from seeing this all semester long for several years. I also asked my students about this last week and (i) nobody said that the error messages are too long, (ii) many said that they do not understand what error messages mean, (iii) one student in each class mentioned that specifically the error message about lack of convergence of a linear solver was helpful (probably the longest error message right now and one that I would not be opposed to put onto the wiki, for example).

So, to me, the conclusion is that we can simply not take our own preferences as the basis for any decisions we take here. If those who participate in this discussion were the basis, we also would not need to have any documentation -- yet, the thing deal.II is most frequently commended for is its extensive documentation, and it is probably the part that several of us spend more time on than anything else. The fact is that most users are not experienced programmers, nor users of numerical methods, and can use our help. So, my conclusion is that we need less cryptic, more explanatory error messages.

Now, could there be error messages that are too long? Yes, of course, and if anyone points one out, we can find solutions to this. For example, we have this in subscriptor.h for one of the most obscure errors we produce:

  DeclException3(ExcInUse,
                 int, char *, std::string &,
                 << "Object of class " << arg2
                 << " is still used by " << arg1 << " other objects."
                 << "\n\n"
                 << "(Additional information: " << arg3 << ")\n\n"
                 << "See the entry in the Frequently Asked Questions of "
                 << "deal.II (linked to from http://www.dealii.org/) for "
                 << "a lot more information on what this error means and "
                 << "how to fix programs in which it happens.");

The explanation at the wiki runs to slightly over 3 pages, and it is clearly the right thing to leave it there. But I don't really see the purpose of splitting the error message from the exception as a general rule. The majority of our users will not immediately know what to do if they get an error, and requiring them to always copy paste a link from the shell into a browser seems like a silly thing to do, even if we can find a way to do this reliably with doxygen. It would benefit a relatively small minority of users, but these are exactly the ones who are not going to spend much time sifting through the output of error messages anyway -- these are the people who will look at the backtrace and know what the problem is, everything else is unimportant. These are also the people most likely to run into errors the least often. So, again, I think this is a solution designed by and for people who are not representative.

I plan on creating a user survey ahead of writing my next deal.II proposal, and I will include questions there about error messages etc. This may give us some more empirical insight, though it'll be only later this or early next year.

@tjhei
Member
tjhei commented Apr 14, 2015

Valid arguments, @bangerth. One advantage for pointing to an online source is that we can update the error messages after the fact and we can even allow discussion/questions about it if it is in a wiki of some sort.

Also, it would be great to have the long descriptions in doxygen. This is independent of how much info is displayed when thrown, but I hate to duplicate the text. This was the main reason for me to not print but link to an online resource. The documentation of the exceptions is pretty bad right now:

  1. seaching for "ExcInUse" doesn't show anything.
  2. If you find it manually in https://www.dealii.org/developer/doxygen/deal.II/group__Exceptions.html the description is useless.
    I am not sure how to fix this because of the macro hackery we are doing.

So, I am okay with keeping the longer error messages.

@bangerth bangerth changed the title from Eradicate exceptions without argument to [Meta] Eradicate exceptions without argument Aug 4, 2015
@tamiko tamiko added the Enhancement label Aug 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment