Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scaled_by_F0 is not used correctly #866

Closed
paulray opened this issue Dec 9, 2020 · 6 comments
Closed

scaled_by_F0 is not used correctly #866

paulray opened this issue Dec 9, 2020 · 6 comments

Comments

@paulray
Copy link
Member

paulray commented Dec 9, 2020

PINT/src/pint/residuals.py

Lines 252 to 256 in 5d4002f

def calc_time_resids(self):
"""Return timing model residuals in time (seconds)."""
if self.phase_resids is None:
self.phase_resids = self.calc_phase_resids()
return (self.phase_resids / self.get_PSR_freq()).to(u.s)

I commented on this in another PR, but scaled_by_F0 has been broken. It seems it was prior to the PR I commented on so I'm repeating here.

That is NOT intended to say whether time or phase residuals are being computed! It is a setting for how time residuals are computed from phases in calc_time_resids(). The default is to do it the way TEMPO/Tempo2 do it and convert from phase to time using model.F0. This is usually a good approximation for many pulsars. However, for some pulsars the current F can be a long way from F0 because of large frequency derivatives. In this case, the conversion should be done using get_PSR_freq(modelF0=False) which is MUCH slower but returns the local frequency using d_phase_d_toa.

@paulray paulray added this to the Tag Version 0.8 milestone Dec 9, 2020
@paulray
Copy link
Member Author

paulray commented Dec 9, 2020

Clearly what is needed is a test that shows the difference, e.g. using a par file with huge F1 (see NGC300 ULX-1).

@aarchiba
Copy link
Contributor

aarchiba commented Dec 9, 2020

I'm inclined to implement this in wideband_improvements (off-topic as it is). But just to check: the idea is that when converting phase to time or vice versa, the choice is between using the model F0 or the spin frequency predicted by the model at the time of the TOA? Both risk producing headaches during fitting, as they introduce an additional dependency of the residuals on F0.

@aarchiba
Copy link
Contributor

aarchiba commented Dec 9, 2020

I'm also worried that the assumption of constant F0 is baked into our process at other stages as well - for example, FFTFIT computes a phase shift, and the conversion to a TOA assumes something about the spin frequency.

@aarchiba
Copy link
Contributor

aarchiba commented Dec 9, 2020

This seems connected to #713 - perhaps I'll just remove the wrong implementation in #861

@aarchiba
Copy link
Contributor

aarchiba commented Dec 9, 2020

Wrong implementation removed. A correct implementation that solves the problem is a bigger challenge and maybe should be addressed under its own bug?

@paulray
Copy link
Member Author

paulray commented Dec 15, 2020

Closing this. Providing options for the phase->time conversion will be left for #713

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants