Re: [arts-dev] About error checking
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
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
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
