Hi Oliver,

I mostly agree with this course of action and your analysis.  And I agree
it is not be necessary to introduce a new error type.  This was a
suggestion to deal with Patrick's speed concerns.  I am more worried about
bad program termination and will not run ARTS with such a flag.

I wish for the opposite of "-DNO_ASSERTS=1" to be added though.  The
asserts in matpack, as you point out, cause a 30% delay in access if we use
them as throws, and most people generally do not want that.  I can,
however, live with this speed reduction in some critical applications I am
running.  I would therefore prefer if these speed critical asserts can also
be changed into logic-errors or some such.  This might be
"-DCATCH_ALL_ASSERTS=1"?  A name suggestion would be
"ARTS_CRITICAL_ASSERT(condition, errmsg)", to try and keep with your naming

I really want all standard asserts out of ARTS.


Den tis 26 mars 2019 kl 11:46 skrev Oliver Lemke <

> 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, right?
> 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?
> Cheers,
> Oliver
> > 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 problem.
> >
> > Bye,
> >
> > Patrick
> >
> >
> >
> > 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 <mailto: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
> physics_funcs.cc:
> >> 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.
> >>    That
> >>    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,
> >> //Richard
> > _______________________________________________
> > arts_dev.mi mailing list
> > arts_dev.mi@lists.uni-hamburg.de
> > https://mailman.rrz.uni-hamburg.de/mailman/listinfo/arts_dev.mi
arts_dev.mi mailing list

Reply via email to