Add support for fill_values functionality with ascii html #5379

tylfin · 2016-10-04T17:48:46Z

taldcroft · 2016-10-05T01:37:46Z

@tylfin - I haven't had a chance to look through this line by line, but overall it looks much better and seems to be on track!

taldcroft · 2016-10-08T19:15:50Z

astropy/io/ascii/html.py

            return None
        return last_index + 1

+    def set_fill_values(self, header, cols, fill_values):


I think you might not need this new method as some of the things being set are probably redundant. At the place where this gets called, self.data is the same as self here. So for instance the self.data object already has a copy of header from __init__. So just try moving these statements inline and then remove one-by-one until things break.

The issue is just that all this code is really intricate and intertwined already (sorry), so working hard to minimize complexity is important.

taldcroft · 2016-10-08T19:17:09Z

astropy/io/ascii/html.py

        cols = list(six.itervalues(table.columns))
+
+        # Set fill_vals on the columns for replacement
+        HTML.data_class().set_fill_values(self.header, cols, self.data.fill_values)


I think you should be able to use self.data instead of HTML.data_class().

taldcroft · 2016-10-08T19:26:13Z

astropy/io/ascii/tests/test_html.py

   </tr>"""
    assert expected in out.getvalue()
+
+def test_write_table_html():


You should also test with a table that has masked values where you set fill_values to [(ascii.masked, 'TEST')] and confirm that the masked table entries are indeed TEST.

taldcroft · 2016-10-08T19:26:58Z

astropy/io/ascii/tests/test_html.py

   </tr>"""
    assert expected in out.getvalue()
+
+def test_write_table_html():


Also test with a table that has a multi-dimensional column, i.e. each row is a 1-d array.

tylfin · 2016-10-10T02:02:17Z

@taldcroft I think I've addressed all the comments you made, tell me if there is anything else!

taldcroft · 2016-10-15T11:12:29Z

@tylfin - this is overall much better again, but I think there is a problem in the masked element handling. Can you add a test along the lines of the example below to make sure it passes?

In [18]: t = Table([[1, 2, 3, 4], ['--', 'a', '--', 'b']], names=('a', 'b'), masked=True)

In [19]: t['a'][0:2] = np.ma.masked

In [20]: t['b'][0:2] = np.ma.masked

In [21]: t
Out[21]: 
<Table masked=True length=4>
  a    b  
int64 str2
----- ----
   --   --
   --   --
    3   --
    4    b

In [22]: ascii.write(t, fill_values=[(ascii.masked, 'MASKED')])
a b
MASKED MASKED
MASKED MASKED
3 --
4 b

Test for fill_values ascii html format issue Test for fill_values ascii html format with optional columns Test for fill_values ascii html format with masked values Test for fill_values ascii html format with multidimensional columns Test for fill_values ascii html format with masked values on multidimensional columns Add fill_values method on HTML that yields either the values themselves, the filled values, or the masked values

tylfin · 2016-10-18T02:15:18Z

@taldcroft Should be taken care of now! Refactored the masked values logic & added the example above as a test.

taldcroft · 2016-10-19T11:11:00Z

@tylfin - looks good, now we just need an update to CHANGES.rst. You should put the following into the 1.0.11 release Bug Fixes section within - io.ascii:

  - Fix a bug where the ``fill_values`` parameter was ignored when writing a
    table to HTML format. [#5379]

taldcroft · 2016-10-21T19:56:02Z

Thanks @tylfin !!

Add support for fill_values functionality with ascii html

eteq · 2016-12-21T07:54:51Z

Unfortunately this PR as-is doesn't seem to be backport-able to 1.0.x, as it uses features that seem to be missing in 1.0.x. A PR directly to the 1.0.x branch addressing this is welcome, though!

ferrigno · 2017-02-16T16:44:40Z

Hello.
When I try to write a HTML table in which a column is made of lists

copy_table.info
Table masked=True length=2080
name dtype shape unit n_bad
"------------------ ------- ------ ------------ -----"
[....]
SPE_PARS float32 (32,) 0

The call to "fill_values" fails with error:
287 list_of_str=ascii.write(copy_table, format="html", fill_values=(ascii.masked, 'None'))
288 #raw_html_cols= ["NAME", "Position reference"]
289

/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/astropy/io/ascii/ui.pyc in write(table, output, format, Writer, fast_writer, **kwargs)
727 return
728
--> 729 lines = writer.write(table)
730
731 # Write the lines to output

/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/astropy/io/ascii/html.pyc in write(self, table)
429 new_cols_escaped.append(col_escaped)
430
--> 431 for row in zip(*col_str_iters):
432 with w.tag('tr'):
433 for el, col_escaped in zip(row, new_cols_escaped):

/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/astropy/io/ascii/html.pyc in fill_values(self, col, col_str_iters)
452 for idx, col_str in enumerate(col_str_iters):
453 if is_masked_column and has_fill_values:
--> 454 if col.mask[idx]:
455 yield col.fill_values[core.masked]
456 continue

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

I am not familiar enough with python and its coding to find a robust solution. I am sorry

tylfin mentioned this pull request Oct 4, 2016

Add support for fill_values functionality with ascii html format #5377

Closed

pllim added io.ascii Affects-release labels Oct 4, 2016

tylfin force-pushed the issue_5354 branch 2 times, most recently from 7bbc8b7 to 8010112 Compare October 4, 2016 19:07

taldcroft self-assigned this Oct 5, 2016

taldcroft reviewed Oct 8, 2016

View reviewed changes

taldcroft added the Bug label Oct 8, 2016

tylfin force-pushed the issue_5354 branch from 8010112 to 4df5fd4 Compare October 10, 2016 02:00

taldcroft mentioned this pull request Oct 15, 2016

Allow round-tripping ASCII masked tables for most formats #5347

Merged

tylfin force-pushed the issue_5354 branch from 4df5fd4 to e839b19 Compare October 18, 2016 02:14

taldcroft added this to the v1.0.11 milestone Oct 19, 2016

Update CHANGES.rst with bug fix for 1.0.11 release

63da166

taldcroft merged commit 1ed6849 into astropy:master Oct 21, 2016

taldcroft added a commit to taldcroft/astropy that referenced this pull request Dec 2, 2016

Remove set_numpy_masked_string hack for HTML writer after astropy#5379

b8b4afb

eteq pushed a commit that referenced this pull request Dec 21, 2016

Merge pull request #5379 from tylfin/issue_5354

8e78871

Add support for fill_values functionality with ascii html

eteq modified the milestones: v1.2.2, v1.0.11 Dec 21, 2016

Uh oh!

Add support for fill_values functionality with ascii html #5379

Add support for fill_values functionality with ascii html #5379

Uh oh!

Conversation

tylfin commented Oct 4, 2016

Uh oh!

taldcroft commented Oct 5, 2016

Uh oh!

taldcroft Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

tylfin commented Oct 10, 2016

Uh oh!

taldcroft commented Oct 15, 2016

Uh oh!

tylfin commented Oct 18, 2016

Uh oh!

taldcroft commented Oct 19, 2016

Uh oh!

taldcroft commented Oct 21, 2016

Uh oh!

eteq commented Dec 21, 2016

Uh oh!

ferrigno commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ferrigno commented Feb 16, 2017 •

edited

Loading