Moved include_gps and include_bipm out of Observatory objects

[paulray]

This is an attempt to clean up handling of include_gps and include_bipm

Rather than having those as attributes of an Observatory object, they are passed as arguments to the site.clock_correction() method. So instead of this:

site = get_observatory("ao", include_bipm=True)
clock_corr = site.clock_corrections(time)

the flow now looks like this:

site = get_observatory("ao")
clock_corr = site.clock_corrections(
        time, include_gps=True, include_bipm=True, bipm_version="BIPM2015"
    )

This has no effect on most users, since get_TOAs() still accepts the same arguments, it just passes include_bipm to the clock correction method rather than the observatory constructor.

In addition, the include_gps and include_bipm arguments now are interpreted as the user wanting UTC times to get the UTC->GPS correction added and TT times to get the TT(BIPM) corrections included. Even if they are True, those corrections will NOT get applied to a time that is already in TDB. This behavior should fix #1691.

The one significant thing on the user side that I've seen so far is that a bunch of observatories (mainly TeV and GW) in observatories.json had include_bipm=False included in their definition (making it their default). This is now lost. I argue this is a good thing since whether or not BIPM corrections are included is not really a feature of the observatory. So, users will now have to specify include_bipm=False if that is the behavior they want when loading TOAs from those observatories.

[codecov]

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 69.27%. Comparing base (178002e) to head (3ee5fdf).
Report is 77 commits behind head on master.

❗ Current head 3ee5fdf differs from pull request most recent head f572167

Please upload reports for the commit f572167 to get more accurate results.

Files	Patch %	Lines
src/pint/observatory/__init__.py	63.63%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1763      +/-   ##
==========================================
- Coverage   69.43%   69.27%   -0.16%     
==========================================
  Files         107      106       -1     
  Lines       24925    24866      -59     
  Branches     4430     4429       -1     
==========================================
- Hits        17306    17227      -79     
- Misses       6510     6525      +15     
- Partials     1109     1114       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[paulray]

Fixes #1766

[paulray]

Also made a fix to the clock_correction() method of TopoObs, where if self._clock is False/None and self.clock_files is True, it would replace the clock corrections with 0.0. I believe this was wrong and it should just add 0.0 to the prior corrections (the BIPM and GPS corrections), so I changed corr = to corr += in that line.

[abhisrkckl]

How does this interact with #1747?

Are clock GPS and BIPM corrections disabled when "CLOCK UNCORR" is given in the par file?

[dlakaplan]

The one significant thing on the user side that I've seen so far is that a bunch of observatories (mainly TeV and GW) in observatories.json had include_bipm=False included in their definition (making it their default). This is now lost. I argue this is a good thing since whether or not BIPM corrections are included is not really a feature of the observatory. So, users will now have to specify include_bipm=False if that is the behavior they want when loading TOAs from those observatories.

Will most users know how/whether to do that? Should the various wrappers to load spacecraft TOAs have that set explicitly now?

[paulray]

How does this interact with #1747?

Are clock GPS and BIPM corrections disabled when "CLOCK UNCORR" is given in the par file?

Should be no problem. #1747 has been merged so is included in this PR. The difference is that the include_bipm flag is now passed to site.apply_clock_corrections() method instead of to the Observatory constructor.

[paulray]

The one significant thing on the user side that I've seen so far is that a bunch of observatories (mainly TeV and GW) in observatories.json had include_bipm=False included in their definition (making it their default). This is now lost. I argue this is a good thing since whether or not BIPM corrections are included is not really a feature of the observatory. So, users will now have to specify include_bipm=False if that is the behavior they want when loading TOAs from those observatories.

Will most users know how/whether to do that? Should the various wrappers to load spacecraft TOAs have that set explicitly now?

Hmm. Most people will hopefully use get_model_and_toas() which will make the right choice based on the setting of CLOCK in your model. Otherwise, it is up to the user. If they call get_TOAs() with no kwargs, they will get the defaults for include_bipm and include_gps, which are True. Maybe there should not be defaults for those? I just don't see how it makes sense for those settings to be part of the observatory. How does the fact that I'm using LIGO or HAWC imply that I don't want a BIPM timescale? I think we just need to educate people by emailing the PINT list and putting info in the release notes.

[dlakaplan]

The one significant thing on the user side that I've seen so far is that a bunch of observatories (mainly TeV and GW) in observatories.json had include_bipm=False included in their definition (making it their default). This is now lost. I argue this is a good thing since whether or not BIPM corrections are included is not really a feature of the observatory. So, users will now have to specify include_bipm=False if that is the behavior they want when loading TOAs from those observatories.

Will most users know how/whether to do that? Should the various wrappers to load spacecraft TOAs have that set explicitly now?

Hmm. Most people will hopefully use get_model_and_toas() which will make the right choice based on the setting of CLOCK in your model. Otherwise, it is up to the user. If they call get_TOAs() with no kwargs, they will get the defaults for include_bipm and include_gps, which are True. Maybe there should not be defaults for those? I just don't see how it makes sense for those settings to be part of the observatory. How does the fact that I'm using LIGO or HAWC imply that I don't want a BIPM timescale? I think we just need to educate people by emailing the PINT list and putting info in the release notes.

If we remove the defaults that would make it more explicit that users need to set this. Maybe that is a good plan. But need to make sure the upstream uses like get_fits_TOAs also do the same.

[paulray]

The problem with removing the defaults is that everyone who has code that calls get_TOAs() without those kwargs will then get an exception. This might be aggravating, but on the other hand it forces people to think about what they really want.

I also looked at what happens when you call get_TOAs() with a model= argument. It can set use_bipm, bipm_version and planets, plus it will set use_gps to False only in the case that CLK UNCORR is in the model.

So really BIPM is associated with the model and it makes sense to me that either its setting should be inherited from the model, or it should force the user to specify it explicitly. On the other hand, I'm now re-thinking the GPS correction. That really is a function of the observatory clock. If the observatory clock corrections put you on UTC(GPS), as is the most common case, then you do want to apply the UTC(GPS) to UTC corrections. Since this is site dependent, probably I should not have taken it out of the Observatory class. The difference is that you should NOT use different settings for BIPM between different sites if you have multi-observatory datasets, but you SHOULD use different settings for GPS for each observatory if some use GPS clocks and some don't.

Do people agree that I should revert my changes to use_gps, but not use_bipm?

[paulray]

In thinking about use_gps more, I changed its name to apply_gps2utc since that better characterizes what it does. It really is a feature of the observatory clock, and just part of the necessary clock corrections to go from raw observatory times to UTC (along with the observatory clock file like pks2gps.clk). So, I think it should not be a kwarg to so many functions like get_TOAs(). If you really want to use an observatory and NOT apply the correction from GPS to UTC that the observatory normally thinks should be done, you could instantiate the site yourself as site = get_observatory('gbt',apply_gps2utc=False). However, this does NOT overwrite the registry so if you later call get_observatory('gbt') you will get the normal version with apply_gps2utc False.

I'd like to hear opinions but I don't think we need to make it so easy to turn off the gps2utc corrections in normal code. That setting should be part of observatories.json and that's basically it. Thoughts?

[paulray]

Ok, question to consider. How easy should it be to not apply the UTC(GPS)->UTC corrections?
Is it sufficient to just need to modify observatories.json to turn it off for any observatory where you don't want it?

Or, if you want to be able to do it programmatically, what is acceptable? It is very painful to make sure that every call to get_observatory() has the kwarg to disable it. But right now when you call get_observatory('gbt',apply_gps2utc=False) it makes a deepcopy of observatory object and then modifies that. This does NOT affect the registry and thus that setting will not "stick" so if you call get_observatory() again without the kwarg, the setting will disappear. It seems to me OK to have a programmer call get_observatory('gbt',apply_gps2utc=False,overwrite=True) once and have that update the registry. Then future calls with no kwargs will get the observatory object with apply_gps2utc=False. But this should be a pretty rare use case since normally you would want this correction for any observatory where it is correct to apply it.

[paulray]

Ah, the deepcopy was added in #1711. I am going to revert that behavior in this PR.

[paulray]

Thanks for all the comments! I have pushed updates to all of them. Hopefully it is good to go now, assuming all test pass.

[paulray]

This is ready to merge. @dlakaplan or @scottransom do you want to give it a look and merge? I addressed @abhisrkckl's comments.

[abhisrkckl]

I'll merge this if it is ready to go.

[paulray]

I'll merge this if it is ready to go.

It would be great to merge soon. I just had to fix several conflicts caused by adding type hints.

[abhisrkckl]

Are you going to push more commits? If not, I'll merge this once the tests pass.

[paulray]

I'm done. Please do merge when tests pass.