Re: [arts-dev] About error checking

[Patrick Eriksson]

Richard,

Now you are a bit all over the place. Yes of course, things can be 
handled in a more nice way if we introduce new features.


ARTS is almost 20 years! When we started ARTS the aim was in fact to use 
as few "groups" as possible. And I would say that we kept that rule a 
long time, but lately things have changed. You have added some new 
groups during the last years and OEM resulted in some other new ones. 
Before we were picky about that each group could be imported and 
exported to files, and could be printed to screen. I don't know if this 
is true for all newer groups.


I don't say that the new groups are bad. For sure we needed a special 
group for covariance matrices, as example. But as usual I would prefer 
that we have a clear strategy for the future. And there should be 
documentation.


I am prepared to discuss this, but not by email. It just takes too much 
time, and these email discussions tend to become a moving target. But I 
could try to find a time for a video/tele conf if there is a general 
feeling that we should add more groups now or in a close future.


Bye,

Patrick






On 2019-03-26 11:51, Richard Larsson wrote:

Hi Patrick,



Den mån 25 mars 2019 kl 19:47 skrev Patrick Eriksson 
mailto:patrick.eriks...@chalmers.se>>:


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.


If a bad z_field can cause an assert today, then it has to be checked 
every-time it is accessed.


This problem seems simply to be a quick and somewhat bad original 
design (hindsight is 20/20, and all that).  To start with, if it has to 
be structured, then z_field is not a field.  It is as much a grid as 
pressure, so the name needs to change.


And since we have so many grids that demands a certain structure, i.e., 
increasing or decreasing values along some axis but perhaps not all, 
then why are these Tensors and Vectors that are inherently 
unstructured?  They could be classes of some Grid or StructuredGrid 
types.  You can easily design a test in such a class that makes sure the 
structure is good after every access that can change a value.  Some 
special access functions, like logspace and linspace, and 
HSE-regridding, might have to added to not trigger the check at a bad 
time, but not many.


Since, I presume, iyIndependentBeamApproximation only takes "const 
Tensor3& z_field" at this point, the current z_field cannot change its 
values inside the function.  However, since it is possible that the 
z_field in iyIndependentBeamApproximation is not the same as the z_field 
when ppath was generated, the size of z_field and ppath both has to 
checked in iyIndependentBeamApproximation and other iy-functions.


However, to repeat: If a bad z_field can cause an assert today, then it 
has to be checked every-time it is accessed.



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.


There were asserts in the physics_funcs.cc functions.  Asserts that were 
triggered.  So I changed them to throw-catch.


I am simply saying that every function needs to be sure it cannot 
trigger any asserts.  Using some global magical Index is not enough to 
ensure that.


A Numeric that is not allowed to be outside a certain domain is a 
runtime or domain error and not an assert.  You either throw such errors 
in physics_funcs.cc, you make every function that takes t_field and 
rtp_temperature check that they are correct, or you create a special 
class just for temperature that enforces a positive value.  The first is 
easier.



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.


I agree, but not by the magic that is *_checkedCalc, since it does not 
guarantee a single thing once in another function.

With hope,
//Richard

___
arts_dev.mi mailing list
arts_dev.mi@lists.uni-hamburg.de

Re: [arts-dev] About error checking

[Richard Larsson]

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
theme.

I really want all standard asserts out of ARTS.

//Richard

Den tis 26 mars 2019 kl 11:46 skrev Oliver Lemke <
oliver.le...@uni-hamburg.de>:

> 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 
> 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 

Re: [arts-dev] About error checking

[Richard Larsson]

Hi Patrick,



Den mån 25 mars 2019 kl 19:47 skrev Patrick Eriksson <
patrick.eriks...@chalmers.se>:

> 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.
>

If a bad z_field can cause an assert today, then it has to be checked
every-time it is accessed.

This problem seems simply to be a quick and somewhat bad original
design (hindsight is 20/20, and all that).  To start with, if it has to be
structured, then z_field is not a field.  It is as much a grid as pressure,
so the name needs to change.

And since we have so many grids that demands a certain structure, i.e.,
increasing or decreasing values along some axis but perhaps not all, then
why are these Tensors and Vectors that are inherently unstructured?  They
could be classes of some Grid or StructuredGrid types.  You can easily
design a test in such a class that makes sure the structure is good after
every access that can change a value.  Some special access functions, like
logspace and linspace, and HSE-regridding, might have to added to not
trigger the check at a bad time, but not many.

Since, I presume, iyIndependentBeamApproximation only takes "const Tensor3&
z_field" at this point, the current z_field cannot change its values inside
the function.  However, since it is possible that the z_field in
iyIndependentBeamApproximation is not the same as the z_field when ppath
was generated, the size of z_field and ppath both has to checked in
iyIndependentBeamApproximation and other iy-functions.

However, to repeat: If a bad z_field can cause an assert today, then it has
to be checked every-time it is accessed.


>
> 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.
>

There were asserts in the physics_funcs.cc functions.  Asserts that were
triggered.  So I changed them to throw-catch.

I am simply saying that every function needs to be sure it cannot trigger
any asserts.  Using some global magical Index is not enough to ensure that.

A Numeric that is not allowed to be outside a certain domain is a runtime
or domain error and not an assert.  You either throw such errors in
physics_funcs.cc, you make every function that takes t_field and
rtp_temperature check that they are correct, or you create a special class
just for temperature that enforces a positive value.  The first is easier.



>
> 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.
>

I agree, but not by the magic that is *_checkedCalc, since it does not
guarantee a single thing once in another function.

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

Re: [arts-dev] About error checking

[Patrick Eriksson]

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 
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 

Re: [arts-dev] About error checking

[Richard Larsson]

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
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

Re: [arts-dev] About error checking

[Patrick Eriksson]

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.


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.


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.


Bye,

Patrick


On 2019-03-22 16:34, Richard Larsson wrote:

Hi all,

I have kept running into problem with errors in ARTS produced by bad 
input for OEM.  Asserts are and not exceptions terminate the program in 
several cases.


I just made a small update to turn several errors affecting Zeeman code 
that before could yield assert-errors into try-catch to throw 
runtime_error().  This means I can catch the errors properly in a python 
try-except block.  The speed of the execution of the central parts of 
the code is unaffected in tests.  I need input from the ARTS developers 
if the way I did this is stylistically acceptable or not.


When updating these error handlers, I decided to use function-try-blocks 
instead of in-lined try-blocks.  I shared some code with Oliver, because 
of the errors above, and he suggested against using function-try-blocks 
and follow the traditional system of keeping all the error handling 
inside the main block.  However, he later in the conversation also 
agreed with me that it makes it much easier to pass errors upwards in 
ARTS from the lower functions if we use function-try-blocks since all 
the function calls of a function are then per automatic inside a 
try-catch block.  So we decided to run the stylistic question by everyone.


Please give me a comment on if this is OK stylistically or not in ARTS. 
I find the function-try-block cleaner since all the error-printing code 
is kept away, but if others disagree it just complicate matters.


The easiest demonstration of this change is in the updated 
src/physics.funcs.cc  file.  Please have a 
look at the two "planck()"-functions.  Both versions only throws (const 
char * e) errors themselves and turns them into std::runtime_error 
before re-throwing.  However, this means that the VectorView version of 
the function can see an error that is (const std::exception& e) because 
the catch-block of the Numeric planck() function turns it into one.  And 
since all errors in ARTS has to be runtime-errors for the user, it can 
also know that any upwards forwarding will deal with runtime-errors.


With hope,
//Richard

The src/physics_funcs.cc planck() error handling:

If the planck() Vector-function is sent a negative temperature, the 
error it produces will look as such:

Errors in calls by *planck* internal function:
Errors raised by *planck* internal function:
     Non-positive temperature

If the planck() Vector function is passed a frequency vector as [-1 -0.5 
0 0.5, 1], the error it produces will look as such:

Errors in calls by *planck* internal function:
Errors raised by *planck* internal function:
     Error: Non-positive frequency
     You have 3 frequency grid points that reports a non-positive frequency!

Ps.  To not have to search.

Function-try-block form:  void fun() try {} catch(...) {}

Inline form: void fun() {try{} catch(...) {}}

Same length of code.  Function-try-blocks do not have the variables 
before the throw-statement available for output, they have to be thrown 
to be seen.  However, you can perform most if not all computations you 
wish inside the catch-block.  Like the error counter I made for f-grid 
in the *planck* function's catch above.



___
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
arts_dev.mi@lists.uni-hamburg.de