Add waveform error handling to Relbin (and other models) #4996
Conversation
| """ | ||
| loglr = sh_total = hh_total = -numpy.inf | ||
| if self.return_sh_hh: | ||
| results = (sh_total, hh_total) |
There was a problem hiding this comment.
I'm not sure it makes sense to catch the case where the return products are different. These should never be called in such a case and you can't be certain you are matching the return product format (e.g. is it a vector or a scalar?). This path should never be used for normal calling of the model (e.g. only reconstruction or interaction with other model which should itself probably decide what to do with its own handler rather than work out how to handle unexpected input.
There was a problem hiding this comment.
I was just trying to match the return signature. What do you suggest doing in this case? Just return -numpy.inf?
There was a problem hiding this comment.
Right, I mean it's a bit more complicated since in many cases it would be a vector for sh_total and hh_total. I'm not sure how clear it would be to handle all such cases (nor if that's really the right thing). Indeed, I think just handling the standard case is fine for now.
There was a problem hiding this comment.
So if return_sh_hh is set to True, should I just raise the error instead of trying to catch it?
There was a problem hiding this comment.
Yeah, I think that case should just be an uncaught error e.g. it probably should blow up on the user since I think they are doing something wrong to end up in that situation.
| @@ -166,8 +169,181 @@ def test_brute_pol_phase_marg(self): | |||
| model.update(**self.q1) | |||
| self.assertAlmostEqual(self.a2, model.loglr, delta=0.002) | |||
|
|
|||
|
|
|||
| class TestWaveformErrors(unittest.TestCase): | |||
There was a problem hiding this comment.
Thank you for adding a unittest!
I think this mostly looks great. There was one minor comment. Otherwise, once you are happy with the PR and assuming all the tests now pass, I think this is good to go.
The
Relativemodel (and models that inherit from it) do not catch failed waveform errors like the GaussianNoise models do. This adds that functionality to those models.To avoid repeating code, and to standardize catching these errors, I created a decorator
catch_waveform_errorthat can be added to any method that will call a waveform generator. The decorator will catch anyNoWaveformError,FailedWaveformError, andRuntimeError. It will then call the class's_nowaveform_handlermethod (for the latter two errors this will only happen if the class'signore_failed_waveformsattribute is set to True). In order for this to work, I had to rename some of the class's_nowaveform_loglrand_nowaveform_loglto_nowaveform_handler.All models that previously caught these errors have been updated to use the decorator, along with the new addition to the Relative model. I added unit tests to test that the models appropriately catch each error.
One slight issue: in order to catch waveform errors in the Relative model, I also had to catch RuntimeErrors. This is being done at the level of the loglr function. This does mean that if a RuntimeError is raised elsewhere in the loglr function (not by the waverform generator), then the
_nowaveform_handlerwill be triggered. I figured this would be ok since it's usually the waveform generator that raise RuntimeErrors.