Allow sets of units to be enabled

[mdboom]

This is a follow on to #1073.

This adds methods add_enabled_units and set_enabled_units to allow for adding sets or individual units after the fact.

I played with various approaches to this for a while. I'm not crazy about the fact that this relies on global state (the present implementation does, too, although less flexibly). The alternative is to require passing in sets of units to functions that do searching (e.g. find_equivalent_units). Ulteimately, I also thought that the convenience of "setting and forgetting" new units is more important, and the lack of thread-safety is not terribly problematic in this case, as the 99% use case is adding units as part of initialization, not adding units "on-the-fly".

There is no function to remove units -- that's tricky to get right giving all of the aliasing and duplication checking that goes on. Instead, I added context managers to allow for temporary enabling of units that restore to the previous state after exiting the with block.

With this change, Imperial units are no longer available by default, and they aren't injected into the astropy.units namespace ever. Most of the changes to the tests are for this.

Also, a note about backward compatibility. The old register and deregister methods now just raise an exception. It isn't really feasible to deprecate them and remove later, since that would require two fundamentally different ways of handling the unit registry at the bottom level at the same time. I hope that registering custom units is enough of a corner case (outside of astropy itself) that this won't be a big issue. We did warn that the API is not final on this stuff in 0.2 after all anyway.

[astrofrog]

I wonder whether it would be nicer for users to simply be able to do

u.enable_imperial()

if there are not too many 'collections' of units, this would be manageable.

[mdboom]

We could add that as an alias, but the more flexible add_enabled_units is still required (for custom units, if nothing else).

[astrofrog]

I like this in principle, though I wonder whether maybe we can make this completely thread-safe by making u a class instance instead of using the whole module so that the content of the module itself isn't changing (though maybe the issue is still the same in terms of threading?).

I wonder whether one could also make it so that units that are not enabled would raise an exception explaining how to enable them, when doing e.g. u.knot?

[astrofrog]

I need to think about the functionality implemented here a bit more from a user point of view. What are we really trying to do here - do we really want things like u.oz to not work, or is it that we don't want these units to play a role in compose/decompose and other similar methods? Is the problem unit clutter, or the fact that some of these units are ill-defined?

[mdboom]

I think the only way to make this threadsafe would be to require a set of units to be passed in to the compose and find_equivalent_units methods. That just seemed too heavy weight from a user's point of view to me. As a middle road, we could add a kwarg that's used, if provided, and if not it uses the global and non-thread-safe set of units.

I'm not sure about the benefit of having u.knot raise an exception explaining how to enable it. The whole point of this is to decrease the size of the namespace.

I think the problem here, as I understand it, is unit clutter.

[mdboom]

We should probably revisit this. I think it feels a little bit like a "toy" that imperial units are included by default. This is one solution. The other is just to remove them outright. Either way, I'd like to improve this for 0.3.

[eteq]

Between those solutions, @mdboom, I personally like a solution along these lines... But I am with @astrofrog that I'm not sure exactly how wide-ranging the enabling/disabling should be. That is, if the problem is namespace clutter, than a solution might be to have the less-common (e.g. imperial) units registered but not in the units namespace. E.g., u.knot wouldn't work, but u.imperial.knot would, as would (7 * u.m).to('knot'). Then the units namespace is clean but conversion remains possible. Or is that not possible for some reason?

[mhvk]

👍 to @eteq's suggestion of u.imperial.knot -- and if strings can be looked up down the tree if they are not found in the main namespace, that would seem good as well (if perhaps running the risk of being slightly confusing, since so far every single unit also has a u.<unit> attribute; but then, for composite units that is not true, so I think this is explainable to new users).

... hmmm, I hadn't actually realised that if I wanted to avoid cgs, I could do import astropy.units.si as u. Neat! (Though it also gets rid of astro-related units, so perhaps not... and strings work like above... ok, never mind, I guess that's why we were discussing enabling/disabling whole groups.)

[mdboom]

@eteq: To clarify: conversion is always possible. You can always convert u.m to u.imperial.knot. The change here is:

1. Imperial units are not included in the toplevel astropy.units namespace
2. Imperial units are not handled by the string parser.
3. Imperial units are not returned when using find_equivalent_units or compose, except after add_enabled_units(u.imperial) is called.

I think (1) is a given -- adding things to a module as the result of a function call is too "magical".

I see your point that (2) should probably work, but in my view, only after add_enabled_units(u.imperial) is called. I think it's actually useful to be strict about what units are available to the string parser. I also think it's more consistent with everything else.

@mhvk: We can't disable some CGS or astrophysical units by default because they are needed to support the minimum requirements of the FITS, CGS and VOUnit standards. This is "astro"py after all, and we should by default support the units required by the community's file formats.

EDIT: (2) does already work correctly with add_enabled_units(u.imperial). It's been a while since I played with this PR.

[mhvk]

@mdboom - I agree with your summary and definitely don't want to disable cgs by default (and certainly not astrophysical units); just thought for a moment I could exclude cgs on my wish, and do see this as a possible advantage of the ability to disable some units in one's own code).

I should let @eteq confirm, but my impression was that he wondered whether with his suggestion the machinery to enable/disable units was needed at all (the unit package is not the easiest to comprehend as it is -- at least to me -- and this PR would seem to make it substantially trickier).

[mdboom]

@mhvk: I'm not sure I follow. We need to define many of the units in cgs and astrophys just to support parsing of FITS and VOTable files, so it's a non-starter to not have them defined in the top-level namespace by default. If you want a namespace with just si, you can, of course, do from astropy.units import si.

Fundamentally, you need to have a "pool" of units somewhere in order to support the find_equivalent_units and compose operations. There are three ways I considered doing this:

1. Importing a module with units would automatically add itself to the pool. This seems too magical -- that merely importing a module would have wide-ranging global side effects.

2. Require the user to provide the set of units as a keyword argument to find_equivalent_units, compose (and probably to string parsing as well). This seems too verbose, and generally, a user will be using the same pool over and over in a particular application/script.

3. The present approach.

I'm not averse to doing a sort of hybrid between (1) and (3), so the user would do:

from astropy.units import imperial
imperial.enable()

which would be equivalent to:

from astropy.units import imperial
u.add_enabled_units(imperial)

[mhvk]

@mdboom - I think we're talking at cross-purposes -- you're right that if I wish I can already import si and astrophysical -- and really this is besides this PR.

I guess the above diluted the only concern I could see, that there might be a case for making what will now be the default the only case, i.e., keep cgs, si, and astrophysical in the main namespace and allow imperial only through u.imperial.<...>. The main advantage would be not introducing additional complexity. That said, I think your solution is quite elegant, so don't take this as a real objection.

Just for my enlightenment, though, if in a given python session, any routine enables u.imperial, will that stay on for other routines that import astropy.units, or will it be temporary?

[mdboom]

I guess the above diluted the only concern I could see, that there might be a case for making what will now be the default the only case, i.e., keep cgs, si, and astrophysical in the main namespace and allow imperial only through u.imperial.<...>.

That's all fine if we never want imperial (or any custom units for that matter) to play with find_equivalent_units or compose. I'm not sure that's an acceptable limitation, hence the additional complexity.

The effect of this is global, so enabling a set of units enables them everywhere. (The lack of global effects is another advantage to proposal (2) above, for what it's worth). However, there are context managers provided here (add_enabled_units_context) to allow:

with add_enabled_units_context(u.imperial):
    # We have imperial units
# No we don't

[mhvk]

OK, fair enough. Since I doubt there will be that many cases of different routines wanting different things and it mattering and them not using the unit context, I think this is a minor worry.

[mdboom]

All that said, I agree it's adding complexity. The simpler solution seems to me to be (1):

1. Importing a module with units would automatically add itself to the pool.

The downside is the "magicalness" (but maybe it doesn't matter in practice), and the inability to remove the units later during the session (which maybe also doesn't matter). What do others think. I'm willing to move to that simpler API if we all agree that being explicit and removing units later don't matter.

[mhvk]

+1 on being explicit, -1 on magic.

[eteq]

Before I was thinking about @mdboom's 1) and 2), but had forgotten about 3). Now that I understand, I definitely like this approach, and am also +1 to explicit. I particularly like @mdboom's

from astropy.units import imperial
imperial.enable()

as the "standard" way to do it.

I would suggest adding a specific mention in the docs of the 1), 2), and 3) that @mdboom gave in response to my last comment, though. That is, very explicitly define what "enabled" means (probably in the "Enabling other units" section this PR adds?)

It might also make sense to make a configuration item for this - let the user provide a list of which units systems (i.e. modules inside units) they want enabled on startup, and have that default to ['cgs', 'astrophys'].

[mhvk]

+1 on the methods, though -1 on a configuration item for the unit systems -- too risky that code starts to depend on such items being set "right". (and of course si has to be in the default -- we should encourage astronomers to follow the advice of their professional organisation!)

[astrofrog]

As @mhvk, I'm +1 on

from astropy.units import imperial
imperial.enable()

but don't like the idea of a configuration item.

[mdboom]

I have added imperial.enable (and updated the docs to put that front-and-center) and rebased. As far as I know this is good to go.

[taldcroft]

Looks good to me from a docs and tests perspective.

[astrofrog]

Same goes for me. @mhvk - maybe you could take this for a spin to see if any unexpected issues pop up?

[taldcroft]

Can #1073 be closed?

[mhvk]

@astrofrog - would love to, but am leaving tomorrow for travel and hence am very unlikely to get to it until after/at astropy.

[mdboom]

@taldcroft: Yes. This completely supercedes #1073.

[eteq]

Perhaps this should be "Enabling Other Unit Systems"?

[eteq]

That is, I realize it can be used to add both individual units and unit systems, but I wouldn't necessarily think to look here to learn about activating something like imperial unless it mentioned unit systems...

[mdboom]

Would "Enabling other units or unit systems" work, or is that too wordy?

[eteq]

Maybe you should add an

Examples
--------

section here? It can just be a copy of what you have in the docs, but a lot of people don't know what "context manager" means, but will understand if you show them a with statement using it.

[mdboom]

That makes sense. There is an example for add_enabled_units -- I can duplicate something similar here.

[eteq]

Looks good to me aside from the comments above.