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

table: change 'units' to 'unit' #1174

Merged
merged 34 commits into from Jul 9, 2013

Conversation

Projects
None yet
7 participants
@adrn
Member

adrn commented Jun 11, 2013

I suspect this will make some people angry, but we need to decide on plural or singular. My vote goes for singular, and that's what we decided on for Quantity and in coordinates. I've changed all the units instances to unit (where appropriate).

@mdboom

This comment has been minimized.

Contributor

mdboom commented Jun 11, 2013

No anger from me. 😉 Haven't really read this line-by-line, but I agree in principle. I know that saying "the units of a value" is common English usage, but in code, I generally don't like to see plural unless something is a collection of something.

@astrofrog

This comment has been minimized.

Member

astrofrog commented Jun 11, 2013

If we do this we should probably deprecate this properly so that it doesn't break in 0.3. Is this the only place the plural appears?

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jun 13, 2013

I'm OK with this, but I do think there should be a deprecation release. Maybe we can make a Column.units property that issues a warning and then returns self.unit? There are some function argument name changes in table (e.g. pformat) that should probably be deprecated, I guess by means of a show_units kwarg that isn't documented and but which issues a warning and sets show_unit accordingly.

There are a number of units in io.ascii that need fixing. Note that the units kwarg in the LaTex class should not be changed since that refers to a dict of unit values. Just quickly scanning the test failures, the first one I saw was in io.fits.

I'm not sure what you mean about a bunch of tests also failing on master. I didn't do a comprehensive scan, but the first five failures in the Travis output were all about a Column object not having a units attribute.

@hamogu

This comment has been minimized.

Member

hamogu commented Jun 17, 2013

Once we are at it, I also suggest changing dtypes to dtype in tab = Table(dtypes = [np.float, 'S4']). After all, in numpy I do array.dtype not array.dtypes even if array is a record array with multiple types.

I think it would be good to do all changes Plural -> Singular in one PR, that's why I bring it up here, even if it does not strictly fit under the title of this issue.

@eteq

This comment has been minimized.

Member

eteq commented Jun 18, 2013

I'm fine with the change to unit as well, but I agree a deprecation release is a important.

@hamogu - I think a monolathic change of doing all plural -> singular throughout astropy is a bit tough because it would touch a lot of different files and lead to many other PRs needing to be rebased (or do you mean just in table? In that case maybe it's fine, I'm not sure exactly how inter-mixed that is). I do agree we should make this all consistent, though (coordinates in particular needs to be made more consistent with other places)

@hamogu

This comment has been minimized.

Member

hamogu commented Jun 19, 2013

Yes, I agree, maybe not everywhere, but everywhere in table - let's
aim for one PR per module to make things singular.

@adrn

This comment has been minimized.

Member

adrn commented Jun 19, 2013

Great, ok, so my action item is to add units back in the relevant places but issue a deprecation warning?

@embray

This comment has been minimized.

Member

embray commented Jun 26, 2013

All you have to do is make units a property that returns self.unit and put a @deprecated decorator on top of it.

@adrn

This comment has been minimized.

Member

adrn commented Jun 26, 2013

Hm, not getting a DeprecationWarning when I call .units -- any idea why?

@adrn

This comment has been minimized.

Member

adrn commented Jun 27, 2013

Ah, right, so Python 2.7 hides DeprecationWarnings by default... ok

@astrofrog

This comment has been minimized.

Member

astrofrog commented Jun 27, 2013

@adrn - yeah, I think that's a little bit silly given that most users will therefore get no warning and things will suddenly break.

@adrn

This comment has been minimized.

Member

adrn commented Jun 27, 2013

Made the change from dtypes -> dtype too.

@embray

This comment has been minimized.

Member

embray commented Jun 27, 2013

@astrofrog I agree. I've been thinking for a while of ensuring that Deprecation Warnings are displayed by default, even on Python 2.7. This can just go somewhere in astropy.__init__. I need to do this on PyFITS too because a lot of people haven't been aware of the deprecation warnings since they're using 2.7 (I do say in the documentation to check for deprecation warnings but who reads the docs?)

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jun 28, 2013

Made the change from dtypes -> dtype too.

Strictly speaking the dtypes arg is a list of dtype specifiers, and that was the reason it was put in as plural. You cannot provide a dtype or dtype-compatible object:

>>> dat = Table([[1], [2]], dtypes=[('a', int), ('b', int)])
ValueError: mismatch in size of old and new data-descriptor
>>> dat = Table([[1], [2]], dtypes=np.dtype([('a', int), ('b', int)]))
ValueError: dtypes must be a list or None
>>> dat = Table([[1], [2]], dtypes=[int, int])
# OK
>>> dat = Table([[1], [2]], dtypes=[np.dtype(int), np.dtype(int)])
# OK

Now maybe the above errors should be fixed, but as-designed there was a specific reason for dtypes instead of dtype as the arg name.

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jun 28, 2013

Interestingly pandas takes a dtype argument, but it doesn't behave like it should, and the corresponding DataFrame attribute is dtypes:

In [49]: x = pd.DataFrame([[1, 2]], dtype=np.dtype([('a', int), ('b', int)]))

In [50]: x
Out[50]: 
        0       1
0  (1, 1)  (2, 2)

In [51]: x.dtypes
Out[51]: 
0    object
1    object
dtype: object

Basically pandas is doing something weird relative to what I'd expect from numpy array.

@hamogu

This comment has been minimized.

Member

hamogu commented Jun 28, 2013

My point here was the comparison with numpy.rec.arrays, where it is also
called dtype (singular), although a more complex objects (a list)
needs to be passed in.

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jun 28, 2013

In the end I agree we should probably change dtypes to dtype since it probably an unnecessary source of confusion. But at the same time I think that the new dtype arg should accept anything that works to initialize np.dtype.

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jun 28, 2013

@adrn - every function or class initializer that previously accepted units or dtypes needs to still work with the plural forms in 0.3, but issue a DeprecationWarning. Basically the rule is that no existing code should break because of this change.

The basic plan is that you need to add back the old (plural) keyword arg at the end of the argument list. If that is specified by name in the call then set unit = units (e.g.) and issue a deprecation warning.

Maybe we could make some sort of keyword argument change decorator that handles the deprecation warning and the argument swap. I guess a decorator could be a good thing also because (I think) we could really hide the fact in the call signature and docs that the plural form is still an acceptable keyword arg. So you would do

@deprecate_keyword_arg('units', 'unit')
def func(arg1, arg2, unit=None, arg3=None):
    pass

@iguananaut - do you know of any available decorators that would fit this bill? It actually seems like a generic thing that has probably already been invented.

@adrn

This comment has been minimized.

Member

adrn commented Jun 28, 2013

Ah, right -- deprecated the attributes but forgot about the kwargs.

@embray

This comment has been minimized.

Member

embray commented Jun 28, 2013

I don't have anything like that on hand for arguments. When I've needed to do that (rarely) I've just hand-coded it the function.

@adrn

This comment has been minimized.

Member

adrn commented Jun 28, 2013

Ok, I think I added the plural kwargs back where necessary, and I added some tests to make sure they raise DeprecatingWarnings.

@adrn

This comment has been minimized.

Member

adrn commented Jun 28, 2013

Rebased on master.

@adrn

This comment has been minimized.

Member

adrn commented Jun 28, 2013

@iguananaut any idea why the build is failing? can't make any sense of that...

@eteq

This comment has been minimized.

Member

eteq commented Jun 29, 2013

@adrn - I think this is unrelated to your changes and is some Travis quirk with the parallel testing. I just restarted the last set of tests and hopefully that'll do it.

Quick note: there should probably be an entry in the changelog about this. Maybe also add this to the section about API breaking, indicating that the API isn't going to break in this version, but will in the next?

@adrn

This comment has been minimized.

Member

adrn commented Jun 29, 2013

Cool, done!

@adrn

This comment has been minimized.

Member

adrn commented Jun 29, 2013

Argh, docs bug...how is such a simple PR such a hassle :)

@astrofrog

This comment has been minimized.

Member

astrofrog commented Jun 30, 2013

@adrn - the issue is in CHANGES.rst:

../CHANGES.rst:112: ERROR: Unexpected indentation.

There were also a few other failed builds, but I don't think that's related to this pull request.

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jun 30, 2013

@adrn - just one final check that you grepped the entire doc tree for units and there were no remaining instances that need to be changed. Otherwise looks good to me, thanks for opening this can of worms and getting it closed again!

@adrn

This comment has been minimized.

Member

adrn commented Jun 30, 2013

@astrofrog thanks for the direct pointer!

@taldcroft ok, double checked and found a few that I forgot to change! fixed.

@adrn

This comment has been minimized.

Member

adrn commented Jun 30, 2013

As noted before, build failures are unrelated.

@adrn

This comment has been minimized.

Member

adrn commented Jul 9, 2013

Someone merge this before I have to rebase again :)

@astrofrog

This comment has been minimized.

Member

astrofrog commented Jul 9, 2013

@taldcroft - good to merge?

@@ -486,7 +486,7 @@ def t(self):
def test_add_none_to_empty_table(self, table_types):
self._setup(table_types)
t = table_types.Table(names=('a', 'b'), dtypes=('i', 'S4'))
t = table.Table(names=('a', 'b'), dtype=('i', 'S4'))

This comment has been minimized.

@taldcroft

taldcroft Jul 9, 2013

Member

I think this somehow got changed from table_types (the fixture that does Masked and Unmasked tables) to the plain table (package).

@@ -496,7 +496,7 @@ def test_add_none_to_empty_table(self, table_types):
def test_add_stuff_to_empty_table(self, table_types):
self._setup(table_types)
t = table_types.Table(names=('a', 'b'), dtypes=('i', 'S8'))
t = table.Table(names=('a', 'b'), dtype=('i', 'S8'))

This comment has been minimized.

@taldcroft

taldcroft Jul 9, 2013

Member

Same issue as above.

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jul 9, 2013

Other than the change above with table_types this looks good.

@adrn

This comment has been minimized.

Member

adrn commented Jul 9, 2013

Oh yea, that's weird... fixed!

@adrn

This comment has been minimized.

Member

adrn commented Jul 9, 2013

Build failure due to network failure I think?

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jul 9, 2013

Well this is probably as close as this will get to success... there is a travis build failure unrelated to testing. Everything else passed, so I'm merging.

taldcroft added a commit that referenced this pull request Jul 9, 2013

Merge pull request #1174 from adrn/table/singular-unit
table: change 'units' to 'unit' and 'dtypes' to 'dtype'

@taldcroft taldcroft merged commit faae7f3 into astropy:master Jul 9, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details
@taldcroft

This comment has been minimized.

Member

taldcroft commented Jul 9, 2013

Thanks @adrn, hopefully you've learned your lesson to never become owner of an issue like this again. 😸

@mdboom

This comment has been minimized.

Contributor

mdboom commented Jul 16, 2013

Sorry to just be getting to this now -- I could have saved everyone a lot of grief: There is a helper function in astropy.tests.helper called catch_warnings that resets all of the warning state first so that the order of tests is not important as to how the warnings come out. This was critical to get things to work when I did all the work to support parallel testing.

See #1265.

@taldcroft

This comment has been minimized.

Member

taldcroft commented Jul 16, 2013

@mdboom - can you please not go on vacation in the future. :-)

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