Hi Oliver,

Here I will only comment on point 1.

The aspect of running ARTS interactively was not pointed out before and then not considered in my answers. With that in mind it seems reasonable to replace most asserts with errors. (Most, as I agree that the index checking in matpack should be left).

So I am favor of your suggestion (without having time to consider the details right now). I have just one wish. Write down some instructions on how the new macros shall be used, and when assert possibly should be applied. We should document these things better in the future.

I suggest to add the instructions to the ARTS developer guide. There is already a small section on use of asserts (sec 1.6).

(To switch to the new macros can be a task for the "ARTS cleaning week" in September!)



On 2019-03-26 11:46, Oliver Lemke wrote:
Hi Patrick and Richard,

I think we're mixing up two issues with error handling in ARTS which should be 
discussed separately:

1. There are instances where ARTS currently exits non-gracefully which is 
painful esp. for python API users.

2. How to better organize where errors are checked and which checks can be 
skipped in release mode.

I will concentrate on point 1 in this email because that's what I think 
triggered the whole discussion. For users who mainly use the ARTS API, asserts 
and uncaught exceptions are painful. Since their occurrence leads to process 
termination, this means in case of the Python API, it will kill the whole 
Python process. This is very annoying if you're working in an interactive shell 
such as ipython, because your whole session will die if this occurs. Therefore, 
our main focus should first of all be on fixing these issues.

Currently, when we catch exceptions at higher levels such as main agenda 
execution, ybatch loop or the ARTS API interface, we only catch exceptions of 
type std::runtime_error. If an unforeseen std::exception from any library 
function is thrown, it'll lead to program termination. This issue is rather 
easy to solve by explicitly catching std::exception in addition to 
std::runtime_error in the appropriate places and handle them just as 
gracefully. I will take care of adding the code where necessary.

For assertions, the fix is a bit more involved. Since they can't be caught, we 
would need to replace them with a new mechanism. As the benchmarks have shown 
we won't lose (much) performance if we use try/catch blocks instead. There are 
very few exceptions such as index operators in matpack which should better be 
left alone.
After discussion yesterday with Stefan, we came up with the following proposal: 
Introduce a couple of convenience methods (macros) that can be used similar to 
how the standard assert works now:

ARTS_ASSERT(condition, errmsg)

This would work the same as 'assert' except that it will be throwing a 
runtime_error in case the condition is not fulfilled. Also, this statement will 
be skipped if ARTS is compiled in release mode. They could also be turned off 
in any configuration by the already existing cmake flag '-DNO_ASSERT=1'. If 
anyone feels that the current name of this option doesn't properly reflect its 
purpose, it can be renamed of course.

ARTS_THROW(condition, errmsg)

This will be the same as ARTS_ASSERT except that will always be active.

Both macros will take care of adding the function name, filename and linenumber 
to the error message.

More complex checks have to be implemented in a custom try/catch block of 
course. And blocks that should be possible to be deactivated should be placed 
inside a preprocessor block such as DEBUG_ONLY. Again here, if the name seems 
inappropriate, another macro that does the same thing with a better fitting 
name could be introduced. So we could have IGNORABLE as an alias to DEBUG_ONLY 
and -DIGNORE_ERRORS=1 as an alias for -DNO_ASSERT=1 . I don't see why we need 
both because in Release mode we would want to activate both options anyway, 

With respect to point 2., I don't see yet a clear strategy or way to give a 
clear definition on which errors should be ignorable and which not. Since the 
past has clearly proven that is already difficult to decide when to use an 
assertion and when a runtime_error, I don't fancy the idea of introducing a 
third class of errors that's defined as 'Can be turned off if the user knows 
what he's doing'. Correct me if I'm wrong Richard, but that's basically what 
you want to achieve with the proposed 'IGNORABLE' flag?


On 25 Mar 2019, at 19:47, Patrick Eriksson <patrick.eriks...@chalmers.se> wrote:

Hi Richard,

I can agree  on that this is not always critical for efficiency as long as the 
check is a simple comparison. But some checks are much more demanding. For 
example, the altitudes in z_field should be strictly increasing. If you have a 
large 3D atmosphere, it will be very costly to repeat this check for every 
single ppath calculation. And should this be checked also in other places where 
z_field is used? For example, if you use iyIndependentBeamApproximation you 
will repeat the check as also the DISORT and RT4 methods should check this, as 
they can be called without providing a ppath.

Further, I don't follow what strategy you propose. The discussion around planck 
indicated that you wanted the checks as far down as possible. But the last 
email seems to indicate that you also want checks higher up, e.g. before 
entering interpolation. I assume we don't want checks on every level. So we 
need to be clear about at what level the checks shall be placed. If not, 
everybody will be lazy and hope that a check somewhere else catches the problem.

In any case, it should be easier to provide informative error messages if 
problems are identified early on. That is, easier to pinpoint the reason to the 



On 2019-03-25 12:24, Richard Larsson wrote:
Hi Patrick,
Just some quick points.
Den sön 24 mars 2019 kl 10:29 skrev Patrick Eriksson <patrick.eriks...@chalmers.se 
    Hi Richard,
    A great initiative. How errors are thrown can for sure be improved. We
    are both lacking such checks (still to many cases where an assert shows
    up instead on a proper error message), and they errors are probably
    implemented inconsistently.
    When it comes to use try/catch, I leave the discussion to others.
    But I must bring up another aspect here, on what level to apply asserts
    and errors. My view is that we have decided that such basic
    functions as
    planck should only contain asserts. For efficiency reasons.
Two things.
First, Oliver tested the speeds here.  The results are random in 
number_density (100 million calls, averaged over 5 runs):
with assert:    0.484s
with try/catch: 0.502s, 3.8% slower than assert
no checks:      0.437s, 9.8% faster than assert
dinvplanckdI (20 million calls, averaged over 5 runs):
with assert:    0.576s
with try/catch: 0.563s, 2.3% faster than assert
no checks:      0.561s, 2.7% faster than assert
but with no notable differences.  (We are not spending any of our time in these functions really, 
so +-10% is nothing.)  One thing that asserts do that are nicer that they are completely gone when 
NDEBUG is set.  We might therefore want to wrap the deeper function-calls in something that removes 
these errors from the compilers view.  We have the DEBUG_ONLY-environments for that, but a negative 
temperature is not a debug-thing.  I suggested to Oliver we introduce a flag that allows us to 
remove some parts or all parts of the error-checking code on the behest of the user.  I do not know 
what to name said flag so the code is readable.  "IGNORABLE()" in ARTS and 
"-DIGNORE_ERRORS=1" in cmake to set the flag that everything in the previous parenthesis 
is not passed to the compiler?  This could be used to generate your 'faster' code but errors would 
just be completely ignored; of course, users would have to be warned that any OS error or memory 
error could still follow...
The second point I have is that I really do not see the points of the asserts 
at all.  Had they allowed the compiler to make guesses, that would be somewhat 
nice.  But in practice, they just barely indicate what the issues are by 
comparing some numbers or indices before terminating a program.  They don't 
offer any solutions, and they should really never ever occur.  I would simply 
ban them from use in ARTS, switch to throws, and allow the user to tell the 
compiler to allow building a properly non-debug-able version of ARTS where all 
errors are ignored as above.
    For a pure forward model run, a negative frequency or temperature would
    come from f_grid and t_field, respectively. We decided to introduce
    special check methods, such as atmfields_checkedCalc, to e.g. catch
    negative temperatures in input.
I think it would be better if we simply removed the *_checkedCalc functions 
entirely (as a demand for executing code; they are still good for sanity 
checkups).  I think they mess up the logic of many things.  Agendas that work 
use these outputs when they don't need them, and the methods have to manually 
check the input anyways because you cannot allow segfaults.  It is not the 
agendas that need these checks.  It is the methods calling these agendas.  And 
they only need to checks for ensuring they have understood what they want to 
do.  And even if the checked value is positive when you reach a function, you 
cannot say in that method if the check was for the data you have now, for data 
sometime long ago in the program flow, or just a bit someone set before calling 
the code.  So all the inputs have to be checked anyways.
For the atmfields_checkedCalc call in particular, it would make more sense to 
have this part of the iy-methods more than anything else.  Since ppath is 
generated externally now (compared to when the iy-methods last had an updated 
error-handling), ppath needs also be checked with the fields themselves anyways 
or you can have segfaults in the interpolations.
    When doing OEM, negative temperatures can pop up after each iteration
    and this should be checked. But not by planck, this should happen on a
    higher level.
    A simple solution here is to include a call of atmfields_checkedCalc
    etc. in inversion_iterate_agenda. The drawback is that some data
    will be
    checked over and over again despite not being changed.
    So it could be discussed if checks should be added to the OEM part.
    data changed in an iteration, should be checked for unphysical values.
    That is, I think there are more things to discuss than you bring up in
    your email. So don't start anything big before we have reached a common
    view here.
Right, I won't.
With hope,
arts_dev.mi mailing list

arts_dev.mi mailing list

Reply via email to