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
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ arts_dev.mi mailing list arts_dev.mi@lists.uni-hamburg.de https://mailman.rrz.uni-hamburg.de/mailman/listinfo/arts_dev.mi
