`pint.observatory` has spooky action at a distance

[abhisrkckl]

The way observatories are read relies heavily on side effects. This makes it very difficult to understand and debug that module unless one already knows what to look for.

[aarchiba]

Are you thinking of the registry/alias map? https://github.com/nanograv/PINT/blob/master/src/pint/observatory/__init__.py#L145 That's there to make it easier to define observatories - no need to list them all somewhere central - but there are less confusing ways to do that.

Or the lazy loading of clock files? That is motivated by performance concerns - clock files can be large, and loading them all can be slow, unnecessarily so when most operations require only one, or at most a few. The pattern of storing something in _variable and having a method _load_variable() is not ideal; one should be able to replace it with @cached_property.

[aarchiba]

Perhaps more of a problem is that various actions - registering the observatories, in particular - are done on import of the relevant modules. This is not nice, but it is not easy to resolve without requiring users to call some setup function before doing anything with observatories. Something could probably be arranged, perhaps by putting load-everything calls in get_observatory and the like.

[abhisrkckl]

I appreciate the need for lazy loading of clock files, and this issue is not about that. I did find that part of the code quite opaque, and more comments will likely be very helpful.

My complaint is about the implementation of the registry/alias map mechanism. It is not at all obvious where and how the registry gets populated, and there is probably a way to make this mechanism less reliant on non-obvious side effects.

More serious is the fact that the get_observatory function rewrites Observatory objects silently, leading to bugs like #1691. (PR #1715 is an attempt at fixing this.)

[aarchiba]

The registry is a pattern used elsewhere in PINT. The idea is that the Observatory class is constructed so that when an observatory object is constructed, it registers itself automatically in a central registry. This registration enables file readers to translate names into Observatory instances.

The pattern is also used for timing models, although in this case the registry is not for instances of TimingModel subclasses but for the subclasses themselves. It exists for the same reason.

This pattern attempts to use python's class construction (or metaclass construction) mechanisms to avoid users having to (and possibly forgetting to) call a registration function whenever they create an object of a certain type. Whether this degree of metaprogramming is helpful or not is unclear. For a library like PINT, the concern is that users would like to simply import PINT and begin doing pulsar timing; if PINT does not work correctly unless the user calls some initialisation function then there is a likelihood that people will have more difficulty using PINT as a library. Unfortunately, this goal requires PINT to do some things that appear magical, either doing its initialisation at import time (which we currently do) or doing some kind of deferred initialisation (worst-case, requiring module-level __getattr__).

This system could be made less magical; in retrospect it was probably an over-enthusiasm for python's metaclass and object-oriented tools that led to the confusing system we now have. I'd be a little hesitant to seriously revise it, since fundamentally people don't expect to have to call any initialisation function before being able to (say) load a .tim file. But it might be possible to simplify the registration process substantially, for example by requiring manual registration unless the observatory was loaded through one of the conventional loading mechanisms.

[paulray]

I'm not sure what a perfect solution to this would be. I think the philosophy should be that there is only one instance of each observatory object and it is kept in the registry. All calls to get_observatory() return that instance. So, if you ask it to modify that observatory (e.g. by changing apply_gps2utc to False), then that will be consistently applied. In other words, observatory characteristics like position, clock correction file, need for gps2utc are global. This is one reason behind moving include_bipm out of the Observatory class, so it isn't carried around with the site forever and in one python session you can work with different BIPM settings/versions.

However, there is a possible failure mode where you want to have an observatory with multiple different settings in one python session. This will cause trouble, since the registry version of the observatory object can get overwritten and then its behavior will change. This argues for Observatory objects being immutable (or as close as possible). So, in #1763 I have removed the GPS correction from most of the user interface. If you really want to turn it off for a site you can, but that should be uncommon since either a site needs that correction or it doesn't. Therefore unless you specifically overwrite it, an Observatory object is created only once and forever after you just get that same object that is kept in the registry.