Adding performance message for Table.read with `memmap=True` by geekypathak21 · Pull Request #9404 · astropy/astropy

geekypathak21 · 2019-10-18T19:06:48Z

Description

This pull request is to address ...

I added an example in performance docs to show an efficient way to use Table.read().

geekypathak21 · 2019-10-18T19:43:57Z

Thanks for suggestions @pllim @bsipocz . I have changed everything as suggested

taldcroft · 2019-10-18T22:08:25Z


    >>> tm.write('tm.ecsv', overwrite=True, serialize_method='data_mask') # doctest: +SKIP
+
+Reading a ``.fits`` in |Table| using


Should be Read a large FITS file in ..., and same further down read a large FITS file. For small files using memmap=True will likely degrade performance.

The text still refers to reading a .fits, that needs to be FITS file.

geekypathak21 · 2019-10-19T09:52:39Z

Thanks, @taldcroft. Everything done as suggested

cdeil · 2019-10-20T10:59:44Z

@himanshupathak21061998 - Thanks!

From this docs addition, I don't think I would have understood when to use memmap=True and when it's not useful.

@saimn explained the key points in #8573 (comment) and showed examples in https://github.com/saimn/misc-astro/blob/master/astropy/Table_memmap.ipynb

FITS data is stored row by row
memmap is only useful when reading a row subset
reading a single column will read all table data (all rows and columns) into memory

Suggest to reword the explanation and recommendation along those lines.

geekypathak21 · 2019-10-22T21:09:29Z

@cdeil Should I also include outputs of imu.start_watching_memory() like in the example.

cdeil · 2019-10-23T11:15:30Z

@himanshupathak21061998 - My suggestion would be to not include a fully worked example, including how to measure with ipython_memory_usage.

I think a short explanation like this contains all the infos?

Read with memmap=True
--------------------------

By default `Table.read` will read the whole table into memory, which will take a lot of memory and can take a lot of time, depending on the table size and file format. In some cases, it's possibly to only read a subset of the table by choosing the option ``memmap=True``.

For FITS binary tables, the data is stored row by row, and it is possible to read only a subset of rows, but reading a full column load the whole table data into memory.

    # Very fast, doesn't actually load table data into memory
    table = Table.read("table.fits", memmap=True)
    #  Fast, will read only first 100 rows:
    table2 = table[:100]
    print(table2) # accessing column data triggers the read
    # Will load all table data / all columns into memory:
    col = table["my_column"]

TODO: what about HDF5? probably does support memmap? useful for row or column subsets?
TODO: what about ASCII? probably never supports memmap and that's fine?

geekypathak21 · 2019-10-24T19:19:26Z

@cdeil I think hdf5 also does not support memmap
I have got this error

TypeError: read_table_hdf5() got an unexpected keyword argument 'memmap'

Code to reproduce

import numpy as np
from astropy.table import Table
tbl = Table({ 'a': np.arange(1e7),
             'b': np.arange(1e7, dtype=float),
             'c': np.arange(1e7, dtype=float)})
tbl.write('test.hdf5', overwrite=True)
table1 = Table.read("test.hdf5",memmap=True)

cdeil

@himanshupathak21061998 - A few typos / grammar suggestions inline. Otherwise I'm happy with this now. Thanks!

I'm not sure if the statement on HDF5 is correct. It might always use memmap under the hood. You could try to find out by measuring memory use when reading a row or column subset, or by reading the code. Or others reviewing the PR here like @taldcroft will know for sure.

cdeil · 2019-10-24T19:22:14Z

+--------------------------
+
+By default :meth:`~astropy.table.Table.read` will read the whole table into memory, which 
+will take a lot of memory and can take a lot of time, depending on the table size and 


will -> can

cdeil · 2019-10-24T19:23:25Z

+    Length = 100 rows
+    >>> col = table["my_column"] # Will load all table into memory
+
+Right now :meth:`~astropy.table.Table.read` does not supports ``memmap=True`` for HDF5 and ASCII 


"Right now" -> "At the moment"

supports -> support

for HDF5 -> for the HDF5

cdeil · 2019-10-24T19:24:04Z


    >>> tm.write('tm.ecsv', overwrite=True, serialize_method='data_mask') # doctest: +SKIP
+
+Read FILE with memmap=True


FILE -> FITS

pllim · 2019-10-24T20:38:56Z

@taldcroft , since you requested changes, can you please give this one another look? Thanks!

saimn · 2019-10-25T02:03:24Z

+
+By default :meth:`~astropy.table.Table.read` will read the whole table into memory, which 
+can take a lot of memory and can take a lot of time, depending on the table size and 
+file format. In some cases, it's possibly to only read a subset of the table by choosing 


Suggested change

file format. In some cases, it's possibly to only read a subset of the table by choosing

file format. In some cases, it's possible to only read a subset of the table by choosing

It might be useful to clarify the "In some cases". I think that the table will always be loaded fully in memory if one column must be converted to another format, i.e. when there is a boolean column, or when some column must be scaled.
This is if I remember correctly the discussions on @astrofrog 's PR, so maybe he remembers better than me ;).

saimn · 2019-10-25T02:05:44Z

+    ...               'b': np.arange(1e7, dtype=float),
+    ...               'c': np.arange(1e7, dtype=float)})
+    >>> tbl.write('test.fits', overwrite=True)
+    >>> table = Table.read("test.fits", memmap=True) # Very fast, doesn't actually load data


Nitpick but it's better to be consistent with the other lines:

Suggested change

>>> table = Table.read("test.fits", memmap=True) # Very fast, doesn't actually load data

>>> table = Table.read('test.fits', memmap=True) # Very fast, doesn't actually load data

saimn · 2019-10-25T02:07:07Z

+    98.0 98.0 98.0
+    99.0 99.0 99.0
+    Length = 100 rows
+    >>> col = table["my_column"] # Will load all table into memory


Suggested change

>>> col = table["my_column"] # Will load all table into memory

>>> col = table['my_column'] # Will load all table into memory

taldcroft · 2019-10-25T21:23:43Z

@pllim @saimn - are you happy enough with this now? Seems good enough to me.

pllim · 2019-10-25T21:29:43Z

I am deferring to @saimn . 😬

saimn · 2019-10-25T21:36:20Z

Yes, looks good to me. Thanks @himanshupathak21061998!

pllim · 2019-10-25T22:34:02Z

Should let the [docs only] CI run instead of [skip ci] before merge.

bsipocz · 2019-10-25T22:39:59Z

The doctest is skipped, so I would say #yolo and merge this now.

pllim

I made a commit of nitpicks and triggered the CI. If it passed (I think it should), then this LGTM. Since @taldcroft and @saimn already unofficially approved it, I am going to officially do it here.

But I'll let it stay open a bit more in case @lglattly wants to copy-edit.

lglattly

Some small copy editing suggestions, looks good to me otherwise!

Co-Authored-By: lglattly <[email protected]>

geekypathak21 · 2019-10-29T10:45:24Z

I think tests failures are unrelated to this pr 😕

The review has been addressed, and reviewer was happy with the PR in his latest comment.

bsipocz · 2019-10-29T18:08:16Z

mpl related CI failures are indeed unrelated, so I'm merging this now given the overall approval above.

Thanks @himanshupathak21061998!

astropy-bot Bot added the table label Oct 18, 2019

pllim reviewed Oct 18, 2019

View reviewed changes

Comment thread docs/table/performance.inc.rst Resolved

pllim added the Docs label Oct 18, 2019

bsipocz added this to the v4.0 milestone Oct 18, 2019

pllim added the no-changelog-entry-needed label Oct 18, 2019

pllim requested review from cdeil, saimn and taldcroft October 18, 2019 19:11

bsipocz reviewed Oct 18, 2019

View reviewed changes

Comment thread docs/table/performance.inc.rst Resolved

bsipocz reviewed Oct 18, 2019

View reviewed changes

Comment thread docs/table/performance.inc.rst Resolved

taldcroft previously requested changes Oct 18, 2019

View reviewed changes

MSeifert04 reviewed Oct 19, 2019

View reviewed changes

Comment thread docs/table/performance.inc.rst Resolved

Adding performance message for Table.read

b408ef9

geekypathak21 force-pushed the add-example branch from 5b27e44 to b408ef9 Compare October 22, 2019 21:04

Adding a section Read file when memmap=True

3343537

cdeil reviewed Oct 24, 2019

View reviewed changes

Removing typos [skip ci]

316bba2

cdeil approved these changes Oct 24, 2019

View reviewed changes

bsipocz requested a review from taldcroft October 24, 2019 20:55

saimn reviewed Oct 25, 2019

View reviewed changes

Apply suggestions [skip ci]

828c48d

Minor formatting edits [docs only]

89ef9f1

pllim requested a review from lglattly October 26, 2019 00:18

pllim approved these changes Oct 26, 2019

View reviewed changes

lglattly reviewed Oct 28, 2019

View reviewed changes

Comment thread docs/table/performance.inc.rst Resolved

Comment thread docs/table/performance.inc.rst Resolved

Apply suggestions from code review

5c54262

Co-Authored-By: lglattly <[email protected]>

bsipocz merged commit 1f4e1b2 into astropy:master Oct 29, 2019

saimn mentioned this pull request Apr 21, 2022

Fix use of memmap with Table.read and add option to deactivate the masking of invalid values #12544

Merged

9 tasks done


		>>> tm.write('tm.ecsv', overwrite=True, serialize_method='data_mask') # doctest: +SKIP

		Reading a ``.fits`` in \|Table\| using


		>>> tm.write('tm.ecsv', overwrite=True, serialize_method='data_mask') # doctest: +SKIP

		Read FILE with memmap=True

	file format. In some cases, it's possibly to only read a subset of the table by choosing
	file format. In some cases, it's possible to only read a subset of the table by choosing

	>>> table = Table.read("test.fits", memmap=True) # Very fast, doesn't actually load data
	>>> table = Table.read('test.fits', memmap=True) # Very fast, doesn't actually load data

	>>> col = table["my_column"] # Will load all table into memory
	>>> col = table['my_column'] # Will load all table into memory

Conversation

geekypathak21 commented Oct 18, 2019

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geekypathak21 commented Oct 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

geekypathak21 commented Oct 19, 2019

Uh oh!

Uh oh!

cdeil commented Oct 20, 2019

Uh oh!

geekypathak21 commented Oct 22, 2019

Uh oh!

cdeil commented Oct 23, 2019

Uh oh!

geekypathak21 commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdeil left a comment

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!

pllim commented Oct 24, 2019

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!

taldcroft commented Oct 25, 2019

Uh oh!

pllim commented Oct 25, 2019

Uh oh!

saimn commented Oct 25, 2019

Uh oh!

pllim commented Oct 25, 2019

Uh oh!

bsipocz commented Oct 25, 2019

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

lglattly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

geekypathak21 commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Oct 29, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

geekypathak21 commented Oct 24, 2019 •

edited

Loading

geekypathak21 commented Oct 29, 2019 •

edited

Loading