Fix format conversion in the fast writer with plain format strings ('.2f') #4517
Conversation
|
Hum, the documentation build failed, it seems to be due to |
|
Taking another look at this, The fact that it is defined in |
|
ping @taldcroft ? |
|
@saimn - sorry for the delay, I'll try to have a look soon. |
|
@saimn - I believe that speed was a driver in writing the duplicate code, although just offhand it doesn't seem like much of that code would really Cythonize effectively so I'm not sure. If you look at http://astropy.readthedocs.org/en/latest/io/ascii/fast_ascii_io.html#writing and the referenced notebook, you can see some performance testing that was done. You could try running this notebook using current master and then with your branch to see what changes (if anything) in the plots. |
|
@taldcroft : results are here https://gist.github.com/saimn/f143251187f7e95fe57f |
|
@saimn - awesome! I love patches that remove unnecessary code and improve things along the way! All this needs now is a bug fix changelog entry for 1.0.9. (If you could try cherry picking this PR onto the 1.0.x branch to make sure there is no conflict that would be good). |
Plain format strings ('.2f') were not supported by the fast writer, whereas it
is by the basic writer:
http://docs.astropy.org/en/latest/table/construct_table.html#format-specifier
The basic writer use `astropy.table.pprint._auto_format_func` to try the
different formatting syntaxes. And the fast writer was reimplementing this in
`cparser.pyx` (`auto_format_func`). The comment on these function is not so
clear about why the function is duplicated (*Mimics pprint._auto_format_func for
non-numpy values.*), and the function does less than
`astropy.table.pprint._auto_format_func` so this commit just use this function.
saimn
force-pushed
the
fast_reader_fmt
branch
from
10a6604
to
9d73932
Compare
saimn
force-pushed
the
fast_reader_fmt
branch
from
9d73932
to
113a5fb
Compare
|
@taldcroft : done, and rebased on master to fix changelog conflicts. It also applies cleanly on 1.0.x and tests pass. |
|
Nothing bad, the coverage build failed because of some error with coveralls.io: |
Fix format conversion in the fast writer with plain format strings ('.2f')
|
Thanks @saimn ! |
Fix format conversion in the fast writer with plain format strings ('.2f')
Fix format conversion in the fast writer with plain format strings ('.2f')
|
@saimn @taldcroft - drat. It looks like this fix doesn't work in py 2.6, revealed when I packported this to 1.0.x and 1.1.x: https://travis-ci.org/astropy/astropy/jobs/112989010 and https://travis-ci.org/astropy/astropy/jobs/112988557 Any thoughts on how to fix this? Or should we instead re-assign this to v1.2 and just accept it in the 1.1.x and 1.0.x branches? |
|
@eteq - I would guess that if you change |
|
@eteq - Sorry about that, I forgot about the need for py2.6 compatibility for old versions. @taldcroft is right, replacing the format string with |
Fix Python 2.6 compatibility for #4517
Fix Python 2.6 compatibility for #4517
Fix Python 2.6 compatibility for #4517
Plain format strings (
'.2f') were not supported by the fast writer, whereas it is by the basic writer: http://docs.astropy.org/en/latest/table/construct_table.html#format-specifierWith the fast writer I get this error:
The basic writer use
astropy.table.pprint._auto_format_functo try the different formatting syntaxes. And the fast writer was reimplementing this incparser.pyx(auto_format_func). The comment on these function is not so clear about why the function is duplicated (_Mimics pprint.auto_format_func for non-numpy values.), and the function does less thanastropy.table.pprint._auto_format_funcso this commit just use this function. But maybe there is a good reason for this duplication ?cc @taldcroft