Sanity checks in fit_toas()

[paulray]

I suggest we consider adding some sanity checks in Fitter.fit_toas() before beginning the actual fit. This would allow us to print sensible error messages for common conditions rather than having fitters crash with hard to understand exceptions. This could include:

• Make sure all mask parameters select at least some TOAs. This would catch the case where you have a fitted JUMP or DMX bin that doesn't have any TOAs in it.
• Is there a way to check that 2 parameters are 100% correlated? For example if a JUMP and DMX bin cover the same set of TOAs, or if all TOAs are JUMPed so the JUMP is 100% correlated with the hidden global phase offset?

[demorest]

I think these are good ideas. For the first point, I think most of the code should go into the timing model, there should be something like TimingModel.sanity_check(toas) that fitters (or whatever else) can call.

For the second point, yes the fitters can/should check for this by examining the results of SVD for small singular values which implies highly covariant params. Right now there is the "threshold" option but if this is not enabled there should probably be an error or warning raised.

[paulray]

@champagne-cmd since you are figuring out maskParameters in your JUMP work, could you also add the first of these sanity checks? You just need to loop over any parameters where is_mask is True and test that model.PARAM.select_toa_mask(toas) is not empty. Once we have at least that functionality is a TimingModel.sanity_check function, other stuff can be added later. We (@luojing1211 and I) just had a discussion on gitter with a student whose fits were crashing with an inscrutable error message that this check would have caught and explained exactly what the problem was.

[champagne-cmd]

I’d be happy to!

On Thu, Jun 18, 2020 at 3:19 PM Paul Ray notifications@github.com wrote:

@champagne-cmd https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fchampagne-cmd&data=02%7C01%7Cchloe.a.champagne%40mail-service-3-mx.vanderbilt.edu%7C6fcdf6111d504c54f20308d813bc73c8%7Cba5a7f39e3be4ab3b45067fa80faecad%7C0%7C0%7C637281047398874511&sdata=%2BvjrpsRYGpAdlV94UlM0s7AzalPFlLzk%2FOy84qkQGY8%3D&reserved=0 since you are figuring out maskParameters in your JUMP work, could you also add the first of these sanity checks? You just need to loop over any parameters where is_mask is True and test that model.PARAM.select_toa_mask(toas) is not empty. Once we have at least that functionality is a TimingModel.sanity_check function, other stuff can be added later. We (@luojing1211 https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fluojing1211&data=02%7C01%7Cchloe.a.champagne%40mail-service-3-mx.vanderbilt.edu%7C6fcdf6111d504c54f20308d813bc73c8%7Cba5a7f39e3be4ab3b45067fa80faecad%7C0%7C0%7C637281047398874511&sdata=OFQcY2qS7Xd26VXv2zBuvRDXnpAdo2xfLn4gEDeCnEE%3D&reserved=0 and I) just had a discussion on gitter with a student whose fits were crashing with an inscrutable error message that this check would have caught and explained exactly what the problem was.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnanograv%2FPINT%2Fissues%2F708%23issuecomment-646257516&data=02%7C01%7Cchloe.a.champagne%40mail-service-3-mx.vanderbilt.edu%7C6fcdf6111d504c54f20308d813bc73c8%7Cba5a7f39e3be4ab3b45067fa80faecad%7C0%7C0%7C637281047398884503&sdata=U1HFtPEMk47aYbcXR8yTbm%2B6x6LGAZ3GomOgUN%2Bidzo%3D&reserved=0, or unsubscribe https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FANAU7OWKMEYLIJA46HPIBNLRXJSCFANCNFSM4NF32M5A&data=02%7C01%7Cchloe.a.champagne%40mail-service-3-mx.vanderbilt.edu%7C6fcdf6111d504c54f20308d813bc73c8%7Cba5a7f39e3be4ab3b45067fa80faecad%7C0%7C0%7C637281047398884503&sdata=VmrGME37J32nNPKguGvOiIf9ZNBk0OVa0vmN%2BDA7yac%3D&reserved=0 .

-- Sincerely, Chloe Champagne Computer Engineering and Applied Math Majors Vanderbilt University, Class of 2022

[champagne-cmd]

Hey @paulray I wrote this sanity check for TimingModel:

def sanity_check(self, toas):
     """Sanity check to ensure all maskParameters select at least one TOA."""
        for maskpar in self.get_params_of_type_top("maskParameter"):
            par = getattr(self, maskpar)
            if par.is_mask and len(par.select_toa_mask(toas)) == 0:
                raise AttributeError(
                    "The maskParameter '%s' has no TOAs selected. " % maskpar
                )

I placed it at the top of each fit_toa function in fitter.py. When I try running the testing suite, 8 of the tests are failing because a parameter called TNEQ1 has more key values than select_toa_mask allows. Do I need to add more requirements for the parameters to test? Or move where the sanity check is called? I can also go ahead and open a PR if that would be easier.

[paulray]

Cool, thanks for doing that! I'm not sure what is up with TNEQ1. Hopefully @demorest or @luojing1211 can explain.