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

WIP: An attempt at allowing the 'TCB' units in PINT #465

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mattpitkin
Copy link
Contributor

I've started an attempt at allowing the inclusion of 'TCB' units in PINT. Hopefully what I've started doing is along the right lines, but any suggestions (including to scrub everything and start again, if it looks wrong) are welcome.

Refs #37 and #151

@luojing1211
Copy link
Member

Hi @mattpitkin thanks for the pull request,
We would like to invite you to our next PINT developer telecon for discussion on this pull request. It will be Tuesday on Feb 19th. Please let me know if you can make it.

@mattpitkin
Copy link
Contributor Author

Hi @luojing1211, thanks for the telecon invite. What time is it at?

@luojing1211
Copy link
Member

luojing1211 commented Feb 5, 2019 via email

@mattpitkin
Copy link
Contributor Author

@luojing1211 I think I should be able to make the call, but send me a reminder closer to the time.

@mattpitkin
Copy link
Contributor Author

The changes now don't break the test suite. However, I've not actually added any tests to check that 'TCB' actually works correctly. I imagine that what I could do would be to generate a set of fake time file with Tempo2 with the default units of TCB, and then have a test that does some comparison of those with PINT.

@mattpitkin
Copy link
Contributor Author

@luojing1211 can you email me (matthew.pitkin "at" glasgow.ac.uk) with information on joining the PINT call later?

@luojing1211
Copy link
Member

@mattpitkin did you get the PINT meeting email?

@mattpitkin
Copy link
Contributor Author

mattpitkin commented Feb 19, 2019 via email

@mattpitkin
Copy link
Contributor Author

I've made some changes as suggestion on the call.

I've also added a test script. It succeeds when I allow it to accept residuals on less than 10 microseconds (the maximum residual is ~5.4 microsecs). The TOAs that I create with the TEMPO2 fake plugin have residuals of 0.1 nanoseconds, so I don't know if I would expect PINT to be closer to TEMPO2 or not. What do others think?

@luojing1211
Copy link
Member

luojing1211 commented Feb 20, 2019 via email

@paulray
Copy link
Member

paulray commented Mar 5, 2019

I suggest generating fake TOAs on a few different timescales and look for signatures in the errors compared to Tempo2. There may be a big annual or diurnal term, which should give a clue. For example, if the code is now looking up the planetary positions in the ephemeris tables using TCB, that will cause an error. Those lookups must be done using TDB units even in the case where the par file is in TCB units since TDB is the independent variable in the planetary ephemerides.

@mattpitkin
Copy link
Contributor Author

It looks like there is a periodic (annually varying) difference between the TCB and TDB values as calculated by TEMPO2 and those computed by astropy, with a magnitude of a few microseconds. This is most likely to be the cause of my error. This is probably due to not including the topocentric correction in the 'default' conversion. I'll try using the _get_T[D/C]B_PINT method with a specified ephemeris.

@mattpitkin
Copy link
Contributor Author

I'll try using the _get_T[D/C]B_PINT method with a specified ephemeris.

It doesn't look like I can do this as the get_gcrs method of the Observatory class isn't actually implemented yet! So, currently it looks like there's no way of getting the topocentric correction. If this is the case I'm not sure how PINT is able to match TEMPO2 to 10 ns precision, as the TopoObs defaults to using the astropy time conversion?

@mattpitkin
Copy link
Contributor Author

Sorry, I can see that the TopoObs class does have a get_gcrs implementation. However, because it doesn't take a grp argument it fails. I'll fix this in this patch.

@paulray
Copy link
Member

paulray commented Mar 6, 2019

If a Time() object has a location specified, then timeobj.tdb should compute the TDB time with the topocentric correction, as far as I understand.

@mattpitkin
Copy link
Contributor Author

If a Time() object has a location specified, then timeobj.tdb should compute the TDB time with the topocentric correction, as far as I understand.

Ah, yes! I hadn't realised that. Taking that into account it looks like astropy and TEMPO2 give the same TDBs to within tens of ns, and TCBs to within hundreds of ns.

@mattpitkin
Copy link
Contributor Author

I think my issue might have been coming about due to me simulations being past the last valid Parkes to GPS conversion value in https://github.com/nanograv/PINT/blob/master/pint/datafiles/time_pks.dat. I'll change my simulation to be within the valid range for that and run again.

@aarchiba
Copy link
Contributor

Does this allow mixing of TDB and TCB units? In particular, flexible conversion of everything from TOAs to model parameters back and forth between TDB and TCB would be a very valuable application of PINT.

@mattpitkin
Copy link
Contributor Author

@aarchiba Unfortunately the patch that I've put together is an either/or thing and doesn't allow conversion of parameters back and forth between equivalents in TDB/TCB.

@matteobachetti
Copy link
Contributor

@paulray @mattpitkin any chance that we can refresh this or maybe try another approach?

@mattpitkin
Copy link
Contributor Author

Yes, I can look into this again. I think I need to look more carefully at how the astropy TCB conversion compares to Tempo2. If there not close enough, PINT will need routines to try and replicate the Tempo2 method (which I know makes use of a lookup file).

@paulray
Copy link
Member

paulray commented Dec 15, 2020

I agree this would be great to have. But, the assumption of TDB is baked in pretty deep in PINT, so I think this will require some discussion, e.g. at PINT telecons, to get this done.

@dlakaplan
Copy link
Contributor

Should this be closed because of #1531 ? That doesn't really do the same thing but in the end it does allow PINT users to do TCB <-> TDB.

@paulray
Copy link
Member

paulray commented Oct 24, 2023

It is already labeled as tabled. It might be useful as reference for a future effort to do this, so maybe leave it open?

@mattpitkin
Copy link
Contributor Author

I'm not sure if/when I'd ever be able to complete this. I never got round to having the in-depth look into how the astropy TCB -> TDB conversion compares directory to the Tempo2 version. I'm happy for this to stay open as a reference or be closed.

@matteobachetti
Copy link
Contributor

I write here because it's still an open issue, although my comment is more about the things that are already implemented.
So, using the machinery from #1531 one can convert simple models, and it works fine. I see, though, that some parameters are explicitly listed among things that are not converted, but would seem easy-ish, like TZRMJD? What is the issue there?

I'm interested in converting a model using SIFUNC terms. For a pulsar requiring ~10us precision, would it be as easy as converting all MJDs from IFUNCnnn, or there are additional issues?

@abhisrkckl
Copy link
Contributor

@matteobachetti TZRMJD is pretty easy to implement. I didn't implement its conversion because I didn't think it made much of a difference, since it only changes the overall phase offset.

SIFUNC is easy to do in theory, but I didn't do it because it involved pairParameters, and they weren't a priority at the time. I will implement their conversion if it is useful.

@abhisrkckl
Copy link
Contributor

#1864

@matteobachetti
Copy link
Contributor

@abhisrkckl thanks for your reply! I'm trying to use some ephemerides from the Fermi archive, and they use TCB and SIFUNC functions. The TZRMJD parameter would also be very useful for those trying to measure the residuals of their TOAs compared to published ephemerides.

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

Successfully merging this pull request may close these issues.

7 participants