Remove prefixes for jupiter/earth/sun masses by eteq · Pull Request #5661 · astropy/astropy

eteq · 2017-01-04T00:55:26Z

This is a possibly-controversial PR with a simple change: it removes the "prefixed" versions of the jupiter/earth/sun masses.

My justification here is that I've never heard anyone speak of a "kiloearth mass" or "milliearth mass". For solar masses it's potentially arguable that "kilosun" or "megasun" should exist in galactic contexts, but I still don't think anyone would say "mega solar mass" even if they do say something like that. And I just encountered someone who was learning about units for the first time and got confused by "mearthMass", thinking it was earth mass, as opposed to "milli-earth mass".

In principal we should deprecate these for a version... but I don't think there's any way to deprecate a unit, is there?

cc @keflavich, as author of c657ca0 which added them for jupiter/earth.

keflavich · 2017-01-04T00:58:20Z

👍 I didn't add the prefixes intentionally, I just c&p'd the M_sun template.

MSeifert04 · 2017-01-04T01:00:39Z

But several people already used them, I dimly remember some issues about MMsun or mMsun.

mhvk · 2017-01-04T10:05:05Z

I agree the prefixes should not be used; we also do not have them on proton mass, etc. (although we do have them on the unified atomic mass unit u).
On the other hand, we obviously do not want people's code to break...

bsipocz · 2017-01-04T12:17:05Z

👍 to remove these, and doing the same for the radii too.

adrn · 2017-01-04T13:54:27Z

I agree these prefixed units should be removed, but share other concerns about breaking existing code. On the other hand, I'm not sure we currently have a way to deprecate units?

pllim · 2017-01-04T16:17:16Z

Indeed, I can't think of an easy way to deprecate just a prefix of a unit for only some units. How about we just straight out remove this for 2.0 release? It is a major version change anyway, so people should expect some non-backward compatible stuff.

eteq · 2017-01-04T23:20:09Z

But several people already used them, I dimly remember some issues about MMsun or mMsun.

@MSeifert04 - do you remember who/what this was from?

eteq · 2017-01-04T23:24:12Z

Also, I'm noticing the failures all seem to be due to VO units. Does anyone understand why these are there? It appears the tests are making sure that all of the prefix are present for all of the "solar" units. I could see this as an argument for keeping the solar units, but I guess I'm lost as to whether or not VO actually expects this somewhere...? (Maybe @pllim or @astrfrog know?)

Alternatively, maybe the prefixes should be valid units, but not have them be in the units namespace?

MSeifert04 · 2017-01-04T23:45:24Z

But several people already used them, I dimly remember some issues about MMsun or mMsun.

@MSeifert04 - do you remember who/what this was from?

@eteq I can only find #5517. Sorry, I thought I already posted it.

MSeifert04 · 2017-01-04T23:46:55Z

after re-reading that issue: It was an example where it confused someone, so maybe that issue is in favor of removing them? 😄

pllim · 2017-01-04T23:53:15Z

Looking at IVOA recommendations v1.0 for VO units, in Table 2, unfortunately, "solar mass" is allowed to have prefix ("s") under "vou" column. Is that something that Astropy supports?

mhvk · 2017-01-06T22:47:20Z

If the IVOA allows prefixes, we should do the same for our VO units (i.e., at least for Msun). Since these are defined separately, in principle this PR should not affect that, but I think that so far the assumption has been that our "generic" units are a superset of all of fits, ogip, and vo (but not of cds, which, therefore, can be used as a units context, unlike the others).

So, perhaps the simplest solution is to keep the prefixes for solar units, but remove them for earth and jupiter mass/radius?

pllim · 2017-01-06T22:54:49Z

So, perhaps the simplest solution is to keep the prefixes for solar units, but remove them for earth and jupiter mass/radius?

But the original confusion in #5517 was about solar mass in the first place...

mhvk · 2017-01-06T22:58:22Z

But the original confusion in #5517 was about solar mass in the first place...

Agreed, but there is something to be said for including all allowed fits, ogip and vo units by default, i.e., staying close to some standard even if we don't quite like it (and I definitely do not like prefixes on solar units...).

eteq · 2017-01-10T16:23:12Z

@mhvk

Since these are defined separately, in principle this PR should not affect that

Can you clarify what you mean by that? It looks to me like they are not defined separately, and that's why this is failing.

Can we maybe have our cake and eat it too, though, by not having the prefixed units in the default namespace, but still having them be defined as units? Then u.<tab> would not show people the units, but u.Unit('msolMass') would still work, right?

mhvk · 2017-01-10T17:58:32Z

It may be possible to disentangle what is in the namespace from what is in the unit registry, but I'm not completely sure we would want that.

As for my "in principle": this referred to the fact that vo and generic could be disentangled, but that so far the assumption, also in our tests, has been that generic is a superset that includes all vo units. I think (but have not tried) that if you just removed the earth and jupiter mass prefixes, all tests should pass. (which is why my tendency would be to do at least that, and worry about solar mass separately).

mhvk · 2017-01-25T17:24:19Z

@eteq - I think we're a bit stuck on the solar mass here, which would require more drastic surgery to get to work. How about at least removing the prefixes for earth and jupiter mass here?

mhvk · 2017-02-10T15:52:20Z

@eteq - further discussion on how to deprecate units is in #5793. However, I still think we should go ahead and just outright remove the prefixed earth and jupiter mass units (for solar mass, I continue to feel we cannot outright do that).

pllim · 2017-03-28T16:45:49Z

@eteq , there is a units.deprecated module now. Please use it.

eteq · 2017-06-13T16:58:36Z

Thanks for the reviews, @pllim and @mhvk - I think I've implemented the changes.

@mhvk - that's right - there's actually no deprecation warning for using this - they just have to be pulled in explicitly by import astropy.units.deprecated or similar.

However, I realize I was having a misunderstanding: I thought having them in the deprecated list would mean that e.g. u.Unit('ysolMass') would work. I agree with @pllim that this is desired so that files that conform to the VOUnit standard can be parsed. It turns out that enable() is required for that to work.

I think the best work-around is to add another unit set just like deprecated, but that is by default enabled. The commits I just put up do that.

Note that I also had to tweak the unit-table-generating docstring machinery to do something useful for this use case. You can check out the end result at http://eteq.github.io/astropy/units/index.html#module-astropy.units.vounit_needed.

(Note that this has changed enough that @pllim and/or @mhvk might want to do a re-review...)

pllim · 2017-06-13T17:21:51Z

+
+  - ``solLum``,``solMass``, and ``solRad`` no longer have  their prefixed units
+    included in the standard units.  If needed, they can still be found in  
+    ``units.vounit_needed``, and are enabled by default. [#5661]


"vounit_needed" sounds very cryptic. Needed for what? "vounit_with_prefix"? "vounit_that_eteq_hates_but_cannot_deprecate"? 😉

I think the fully descriptive name would be required_by_the_vounit_standard_but_not_actually_ever_used_by_an_astronomer_in_real_life_so_would_be_confusing_to_have_in_the_main_namespace. But that's 141 characters, which is > 80 😉.

Maybe required_for_vounit? Or prefixed_vounits?

I'll try going with required_by_vounit . I favor that over vounit_with_prefix only because it's possible that in the future we might put non-prefix units there (if there's ever a VOUnit 2.0 or another change in what people think should be in astropy.units).

I like prefixed_vounit... @mhvk ?

@eteq , these are not the only units in VO standards though. Naming these "required by VO" implies others are not required, unless you want to put all VO units here. It's a can 'o worms.

Hmm, I see that. It's really required_by_vounit_buy_not_in_units, but that's still awkwardly long.

pllim · 2017-06-13T17:22:49Z

 # This generates a docstring for this module that describes all of the
 # standard units defined here.
 from .utils import generate_unit_summary as _generate_unit_summary  # noqa
+from .utils import generate_prefixonly_unit_summary as _generate_prefixonly_unit_summary  # noqa


Why import this here?

Just following what that module is already doing. I know some of this stuff is full of circular-import landmines, so I'm guessing that's the reason? And it's not really performance-sensitive so it only gets run once at import-time, practically speaking.

This is imported but not used, so unnecessary for this module? Unless you forgot to call it below?

oops! Good catch - it is indeed supposed to be called below. Looks like I did that locally but forgot to commit this file with the second line...

Is the noqa necessary now it is actually used?

I just copied it from the line above. I actually don't know why it was in the previous line - I'm guessing at least someone's PEP8 or similar checker doesn't like the as style or something? I figured safer to be consistent with the line just above.

I think we need it because the import is not at the top of module.

That's true for si.py as well...

pllim · 2017-06-13T17:23:21Z

    @staticmethod
    def _generate_unit_names():
        from ... import units as u
+        from ...units import deprecated as u_deprecated


Do you also have to import "vounit_needed" here?

yep, that's why the tests were failing - in fact it's supposed to be only that

pllim · 2017-06-13T17:25:08Z

+     - {1} prefixes
+     - {2}
+     - {3}
+     - Only


What does "only" mean? Other tables have "yes" or "no". Just by looking at the rendered doc, "only" is cryptic.

I struggled with this. It's meant to mean "the prefixes, but not the base unit." In the rendered docs you'll see that other columns also pretty clearly say "only the prefixes". I guess I could just leave this column blank for the prefix-only ones? Do you think that would be clearer, @pllim?

I am inclined with just "yes". As far as user is concerned, it is still a unit but with prefix enabled.

pllim · 2017-06-13T17:29:15Z

+# -*- coding: utf-8 -*-
+# Licensed under a 3-clause BSD style license - see LICENSE.rst
+"""
+This package defines units required by the VOUnit standard but that are likely


Maybe there needs to be a note (can be a comment line if you want) to explain why this could not be in deprecated, so people don't attempt that in the future.

Good point! Will add.

pllim · 2017-06-13T17:33:28Z

Also, I don't see test yet. Do we not need test?

Failed on Appveyor, so cancelling Travis:

____ ERROR collecting lib.win-amd64-2.7/astropy/units/tests/test_format.py ____
astropy\units\tests\test_format.py:175: in <module>
    val for key, val in u_format.VOUnit._units.items()
astropy\utils\decorators.py:648: in __get__
    val = self.fget.__wrapped__(objtype)
astropy\units\format\generic.py:85: in _units
    return cls._all_units[0]
astropy\utils\decorators.py:648: in __get__
    val = self.fget.__wrapped__(objtype)
astropy\units\format\generic.py:81: in _all_units
    return cls._generate_unit_names()
astropy\units\format\vounit.py:81: in _generate_unit_names
    do_defines(bases, si_prefixes, ['pct', 'pcount', 'yd'])
astropy\units\format\vounit.py:77: in do_defines
    names[key] = getattr(uwithekey, key)
E   AttributeError: 'module' object has no attribute 'ysolLum'
____ ERROR collecting lib.win-amd64-2.7/astropy/units/tests/test_format.py ____
astropy\units\tests\test_format.py:175: in <module>
    val for key, val in u_format.VOUnit._units.items()
astropy\utils\decorators.py:648: in __get__
    val = self.fget.__wrapped__(objtype)
astropy\units\format\generic.py:85: in _units
    return cls._all_units[0]
astropy\utils\decorators.py:648: in __get__
    val = self.fget.__wrapped__(objtype)
astropy\units\format\generic.py:81: in _all_units
    return cls._generate_unit_names()
astropy\units\format\vounit.py:81: in _generate_unit_names
    do_defines(bases, si_prefixes, ['pct', 'pcount', 'yd'])
astropy\units\format\vounit.py:77: in do_defines
    names[key] = getattr(uwithekey, key)
E   AttributeError: 'module' object has no attribute 'ysolLum'
============================== warnings summary ===============================

pllim · 2017-06-13T17:35:30Z

                        continue
-                    names[key] = getattr(u, key)
+
+                    uwithekey = u if hasattr(u, key) else u_deprecated


uwithkey or uwiththekey are more correct. 😉

The things I do for <80 chars... ;)

Aha! In fixing the test failure I managed to shrink this down so it can be in the following line and stay <80. So no need to worry about the spelling at all! 😛

eteq · 2017-06-13T19:11:26Z

Just pushed up some changes that I think resolve @pllim's comments (although we shall see if the tests pass...)

The one outstanding question is from #5661 (comment) (which got clobbered by an edit) - basically, it's "what's the right name for the module holding the things VOUnit requires but we don't want in the base namespace". In the current version it's required_by_vounit, but I pushed up a branch here that is the same as this one but with an additional commit that changes it to prefixed_vounit. If @mhvk agrees with @pllim that this is better, feel free to use that version instead (If you have set up a remote called eteq pointing to my branch, you can do git push eteq deprefixmasses-prefixed-vounit:deprefixmasses and it should immediately update this PR and it'll be ready to merge.)

mhvk

Yet more nitpicks, sorry that this has become quite a bit more work than imagined! But I like the solution very much. Also think the current name is OK, as are the docs.

mhvk · 2017-06-13T19:34:47Z

 # This generates a docstring for this module that describes all of the
 # standard units defined here.
 from .utils import generate_unit_summary as _generate_unit_summary  # noqa
+from .utils import generate_prefixonly_unit_summary as _generate_prefixonly_unit_summary  # noqa


Is the noqa necessary now it is actually used?

mhvk · 2017-06-13T19:35:47Z

+# -*- coding: utf-8 -*-
+# Licensed under a 3-clause BSD style license - see LICENSE.rst
+"""
+This package defines SI prefix ed units that are required by the VOUnit standard


"prefix ed" -> "prefixed"

Actually, the text reads a bit oddly. How about the following to replace the first
two paragraphs:

This package defines SI prefixed units that are required by the VOUnit standard but that are rarely used in practice and liable to lead to confusion (such as ``msolMass`` for milli-solar mass). They are in a separate module from ``astropy.units.deprecated`` because they need to be enabled by default for ``astropy.units`` to parse compliant VOUnit strings. As a result, e.g., ``Unit('msolMass')`` will just work, but to access the unit directly one has to use ``astropy.units.required_by_vounit.msolMass`` instead of the more usual route possible for the non-prefixed unit, ``astropy.units.solMass``.

mhvk · 2017-06-13T19:44:44Z

+
+# This generates a docstring for this module that describes all of the
+# standard units defined here.
+from .utils import generate_unit_summary as _generate_unit_summary  # noqa


again confused by the # noqa is needed here; it is not there in, e.g., si.py and the routines are used.

Note that the two imports can be combined: from .utils import (... as ..., ... as ...)

Hadn't noticed it's in si.py. So I'm going to guess this is a hold-over from something in the past/someone's overly picky editor, so will remove them.

mhvk · 2017-06-13T19:45:23Z

+
+def _enable():
+    """
+    Enable deprecated units so they appear in results of


Oops, copy & paste error: these are not deprecated units...

mhvk · 2017-06-13T19:45:44Z

+    `~astropy.units.UnitBase.find_equivalent_units` and
+    `~astropy.units.UnitBase.compose`.
+
+    This may be used with the ``with`` statement to enable deprecated


Probably should remove this, since these are enabled by default.

mhvk · 2017-06-13T19:47:49Z

-from .. import deprecated
+from .. import deprecated, required_by_vounit
 from ... import units as u
 from ...tests.helper import pytest  # TODO: Stop using bundled pytest


Remove this while you're at it? Or is this just because your PRs are based on a relatively old branch?

nope, it's that way in master. Will fix.

mhvk · 2017-06-13T19:48:36Z

+    # the purposes of this test.
+
+    with pytest.raises(AttributeError):
+        #nano-solar mass/rad/lum shouldn't be in the base unit namespace


Space after #

eteq · 2017-06-13T22:03:35Z

OK, I think I've addressed @mhvk's comments - assuming the tests pass I believe this is now good to merge (unless @pllim can come up with a compromise name without the baggage of required_by or prefix...? ).

mhvk · 2017-06-13T23:09:36Z

I'm fine with the name, but let me throw out vo_prefix_units -- That would seem more neutral.

eteq · 2017-06-14T03:31:32Z

vo_prefix_units still has the problem that it's restricted to prefixes, and it's possible that in the future other units that are not just prefixes might be included (but I'm receptive to the argument that that's low enough probability that we should ignore it if that's either of your stances...)

pllim · 2017-06-14T14:39:25Z

vo_thingies? misc_vounit? Anyway, I don't have any good suggestion either, so I guess this is okay to merge when you resolve the change log conflict. We can revisit if people somehow uses that module and complain about the name...

eteq · 2017-06-16T16:21:02Z

Used the fancy web conflict resolution thingie for the first time... worked well, so merging!

eteq · 2017-06-16T16:21:56Z

(Also the last commit's errors aren't real - I killed the travis job so that our queue won't be so jammed up)

bsipocz · 2017-06-16T16:24:15Z

@eteq - Why is that last one a merge commit?

mhvk · 2017-06-16T16:38:39Z

Great! (but github adding merge commits perhaps a bit less great, though perhaps it doesn't matter?)

pllim · 2017-06-19T15:45:20Z

github adding merge commits

I guess we should avoid "fancy web conflict resolution thingie" in the future?

bsipocz · 2017-06-19T15:48:24Z

Or put bigger pressure to GH to fix it and finally offer a rebase button ;)

In pull request astropy#5661 it was decided that the prefixed versions of `solLum`, `solMass` and `solRad` units should not be present in the main `units` namespace. However, they are valid units in the VOUnit format, so they are defined in a separate `required_by_vounit` module. Furthermore, because the `"generic"` format is supposed to be a superset of the other formats then parsing those units should work with the `"generic"` format too. Previously the `required_by_vounit` module added its units to the registry the `"generic"` format uses, but that was a side-effect of `required_by_vounit` being imported, which happened by using the `"vounit"` format. Now the `"generic"` formatter can be used to parse those units even if `"vounit"` has not been used yet.

eteq added the units label Jan 4, 2017

eteq requested review from adrn and mhvk January 4, 2017 00:55

eteq added this to the v2.0.0 milestone Jan 4, 2017

mhvk mentioned this pull request Feb 10, 2017

Deprecate ambiguous units #5793

Closed

pllim mentioned this pull request Mar 23, 2017

Deprecate emu unit #5906

Merged

eteq added 5 commits June 13, 2017 00:13

remove prefixes for jupiter/earth/sun masses

6406a82

add changelog entry

021a920

also remove radii

f80a109

make solmass units deprecated

4974ceb

make the other sol/earth/jup mass/radii prefixes deprecated

3a9c379

update changelog for new situation

0b72a09

pllim reviewed Jun 13, 2017

View reviewed changes

eteq added 5 commits June 13, 2017 14:13

tweaks and clarifications from review comments

a8fdade

fixes for tailing tests

1d40567

rename vounit_needed to required_by_vounit

e3e19d8

fix missing call to _generate_prefixonly_unit_summary

bb37a29

add test of required_by_vounit and added deprecated units

85cc1f8

mhvk reviewed Jun 13, 2017

View reviewed changes

eteq added 2 commits June 13, 2017 17:38

fix doc typo

c240be5

updates from @mhvk review

19e5e24

eteq force-pushed the deprefixmasses branch from 9d03e47 to 19e5e24 Compare June 13, 2017 21:53

Merge branch 'master' into deprefixmasses

04bae9a

eteq merged commit f451081 into astropy:master Jun 16, 2017

eteq deleted the deprefixmasses branch June 16, 2017 16:21

eerovaher mentioned this pull request Mar 10, 2025

Enable units from required_by_vounit properly #17868

Merged

1 task done

Conversation

eteq commented Jan 4, 2017

Uh oh!

keflavich commented Jan 4, 2017

Uh oh!

MSeifert04 commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jan 4, 2017

Uh oh!

bsipocz commented Jan 4, 2017

Uh oh!

adrn commented Jan 4, 2017

Uh oh!

pllim commented Jan 4, 2017

Uh oh!

eteq commented Jan 4, 2017

Uh oh!

eteq commented Jan 4, 2017

Uh oh!

MSeifert04 commented Jan 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSeifert04 commented Jan 4, 2017

Uh oh!

pllim commented Jan 4, 2017

Uh oh!

mhvk commented Jan 6, 2017

Uh oh!

pllim commented Jan 6, 2017

Uh oh!

mhvk commented Jan 6, 2017

Uh oh!

eteq commented Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jan 10, 2017

Uh oh!

mhvk commented Jan 25, 2017

Uh oh!

mhvk commented Feb 10, 2017 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Mar 28, 2017

Uh oh!

eteq commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eteq Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Jan 4, 2017 •

edited

Loading

MSeifert04 commented Jan 4, 2017 •

edited

Loading

eteq commented Jan 10, 2017 •

edited

Loading

mhvk commented Feb 10, 2017 •

edited by pllim

Loading

eteq commented Jun 13, 2017 •

edited

Loading

eteq Jun 13, 2017 •

edited

Loading

eteq Jun 13, 2017 •

edited

Loading