Simplify table column format definition? - #2898

Would we retain back compatibility for both other formats? I do have code that sets the format.

I don't see an issue with retaining backwards compatibility, and especially the '%5.2f' format is something I can see that people would do naively too. I think we can easily keep those options and not slow down things much If in the non-callable path in _auto_format_func one were to do something like :

try:
    out = format(val.tolist(), format_)
except ValueError:
    try:
        out = format_.format(val.tolist())
     [etc]

OK, as long as there is no possible ambiguity between the new simplified format and the previous ones, then I'd be good with going ahead with this.

@taldcroft - OK, this is now a PR. @hamogu - you may want to check whether this would take care of #2896 as well - I did try to update the documentation.

Yes, that extra example is exactly what I am looking for. I like the code change that saved a lot of {}: ,too.

Looks like there are some bona fide test failures for Python 3. On visual inspection I can't see why the test should be failing.

Hmm, very strange that I don't get that locally. However, the underlying problem is clear (and hopefully solved with the updated PR): with a masked table and a format set, one tries to format np.ma.masked with a numerical format specifier:

import numpy as np
format(np.ma.masked, '4.3f')
# TypeError: non-empty format string passed to object.__format__

Indeed, this is a problem generally in astropy (it just escaped testing before... now lots of tests added):

from astropy.table import Table
data_rows = [(1, 2.0, 'x'),
             (4, 5.0, 'y'),
             (5, 8.2, 'z')]
t = Table(rows=data_rows, names=('a', 'b', 'c'), meta={'name': 'first table'}, masked=True)
t['b'].format = '{:7.3f}'
print(t)
# a     b     c 
#--- ------- ---
#  1   2.000   x
#  4   5.000   y
#  5   8.200   z
t['b'].mask[0] = True
print(t)
# ... ValueError: Unknown format code 'f' for object of type 'str'

@taldcroft - OK, travis passes now. But please do check again, since the new PR comes with somewhat more intrusive changes to ensure np.ma.masked is processed correctly.

@mhvk - gotta run, will look at this later, but did you just fix #1026? That would be good.

Yes, this fixes #1026; the test cases I added are quite similar to the first example there.

@mhvk - I've reviewed this and I don't see any logic problems. Somehow I feel like it might be possible to refactor the different cases into a single common basis for a cleaner code, but that's outside the scope of this PR. In any case I mostly just suggested adding comments so it's a bit easier to understand the flow. Other than that I'm good with this.

And thanks for doing this, in particular picking up the long-standing #1026!

Also there is the usual CHANGES.rst update, which in this case probably needs two entries, one for the new feature (added simpler way of specifying column format) and bug fix #1026.

@taldcroft - thanks for the comments. Like you, I did feel that the code could be refactored, so I now did that, by iterating over the possible string-based format functions. I also checked speeds, and the tolist() function call itself was most costly. So, I now only use that conversion if numpy <= 1.6 (hopefully, got that OK; travis will telll... [did add a test for it]).

All this means that it is probably wise to have another look...

@embray - this PR now combines a bug fix [#1026] with a new feature. Your call whether it is for 1.0 or 0.4.2 (it is possible to disentangle the changes, though a bit of a pain).

@mhvk - Wow, this is a great improvement! Very clever with the generator function. I'm good to go with this.

@embray - if we can bend the rules just a bit and get this into 0.4.2 that would be nice. The enhancement part of this PR is strictly a back-compatible addition to the API. Of course one must confess that the bug being fixed has been around for a while and there have not been a flood of complaints...

I have no problem with it.

@mhvk - since this can go in 0.4.2 you need to update CHANGES.rst accordingly. In the 0.4.2 section there is something like "Other changes and additions", which is presumably where the format enhancement comment should go. @embray?

Sure.

@taldcroft, @embray - OK, I moved the descriptions in the changelog to 0.4.2. Will merge now.

This is marked "affects-dev" and "affects-release". Only one of these should be used on an issue (for the way I'm using them to help organize issues). For now I'm going to assume the latter.

(To clarify, "affects-dev" should be read as "affects dev only"--that is, this is not an issue that applies to any released version of Astropy).