Fix use of memmap with Table.read and add option to deactivate the masking of invalid values by saimn · Pull Request #12544 · astropy/astropy

saimn · 2021-11-30T16:14:01Z

Since #11222 (v4.3) using memmap with Table.read is no more possible since the data is loaded to look for NaNs in float columns and empty strings in string columns. I think adding an option to avoid those checks would be useful, though I'm not sure what should be the default value. True is best for compatibility with v4.3+ and people may expect Astropy to mask by default. False is best for memory usage and performance on big tables.

Update: the PR adds a mask_invalid option and deactivates the masking when using memmap, also fix #12981 which is asking for a similar option.

Description

This pull request is to address ...

Fixes #

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

pllim · 2021-11-30T16:27:01Z

There is already conflict. Is your branch very old?

saimn · 2021-11-30T17:06:07Z

It's from 2 weeks ago, but there was the unit_parse_strict PR in between. I will fix that later, not critical for now since we need to decide what to do first.

mhvk · 2021-11-30T17:51:59Z

My sense is that if the user passes in memmap there is a clear expectation of what should happen, and masking should not get in the way of that (at least by default). So perhaps in the code the mask could be determined only if not memmap? Or is that too strong? (I guess ideally we'd wrap memmap to do the mask-creation on the fly... MaskedMemmap?)

But I'm also wondering a bit what changed in #11222 that actually broke the previous behaviour - it looks like also before all the data was loaded if there is any column with col.null is not None.

saimn · 2021-12-02T18:02:22Z

Having to use memmap=True to make the code avoid to mask columns seems a bit weird. Also there may be other use cases where one would prefer to avoid the performance penalty of masking columns, so a more specific (and explicit) parameter looks better.
Ideally I would prefer if the masking wasn't done by default (and should have raised this in #11222), since I prefer to activate additional features instead of the opposite. However this is done by default since 4.3, I'm not sure.

The TNULL case (col.null) is a bit different. Many files don't use those keywords, and if the keyword is present then it makes sense to mask unconditionally since it's a FITS feature, reading the file correctly means taking TNULL into account.

mhvk · 2021-12-02T18:40:28Z

OK, reading your comment, I now definitely agree with explicit being better! In principle, one could still let the default of whether or not to mask depend on memmap.

But I'm still confused what in #11222 actually caused a change in behaviour (at least for Table.read(), which you mention on top; for QTable.read() masked columns previously did not exist, but #11222 served to make its behaviour consistent with Table.read()).

saimn · 2022-03-24T21:07:24Z

@mhvk - Updating this PR since the issue was also raised in #12981.

But I'm still confused what in #11222 actually caused a change in behaviour (at least for Table.read(), which you mention on top; for QTable.read() masked columns previously did not exist, but #11222 served to make its behaviour consistent with Table.read()).

As we discussed above, before #11222 Table.read was only masking a column is TNULL is set, and #11222 added a masking of NaNs and empty strings: https://github.com/astropy/astropy/pull/11222/files#diff-2b263cbbb7d9b3215b1a7180dcd1b909c7ee9f78cf1e2e53834ee3b4d6b837b3R248-R251
But maybe I'm missing something or misunderstood your comment ?

saimn · 2022-04-12T19:43:29Z

ping @mhvk (also see #12981 if you missed it)

taldcroft · 2022-04-20T21:37:48Z

@saimn - I'm not a FITS expert but the new arg and code certainly do stop that masking code from executing. If this will solve the issue of memmap not working then I'm willing to sign off on this API change.

The PR needs work, docstring update, narrative docs that indicate this use case, some functional testing or unit tests to prove that the change does indeed fix the memmap issue, etc. (Also for the record update the PR title and description to be more like the PR that solves the problem instead of an issue reporting it.)

Thanks for working on this, it would be good to get for this release since this seems like a pretty bad issue.

taldcroft · 2022-04-20T21:43:41Z

 def read_table_fits(input, hdu=None, astropy_native=False, memmap=False,
-                    character_as_bytes=True, unit_parse_strict='warn'):
+                    character_as_bytes=True, unit_parse_strict='warn',
+                    mask_invalid=True):


Is it worth considering making mask_invalid default to False if memmap is True?

I'm not sure, it can be unexpected to get an unmasked table when using memmap=True. Maybe a warning if memmap=True and mask_invalid=True to explain the user why they also need to set mask_invalid=False ?

Both seem to have advantages: I like the clarity of the warning in that users do not get unexpected data, but worry slightly about read_table_fits being done well inside code where a user may not have access.

I guess eventually we could have a memmap='if_possible'...

I do worry a bit how this function is getting more and more arguments about arguably small differences in how to handle specific items. I wonder if this is the best way to handle them. But that's for another discussion...

The interest of using memmap here is limited to quite specific cases (reading a subset of the rows, #9404, #8573 (comment)), so I don't think a warning would annoy a lot of people. But it's also true if we set mask_invalid=False when using memmap, so maybe it's a good option as well.

I'll let you make the call -- I cannot decide even which option I think is better...

Ok, I went for the first option, setting mask_invalid=False when using memmap, and added some explanations in the docstring.

saimn · 2022-04-21T20:50:14Z

The PR needs work, docstring update, narrative docs that indicate this use case, some functional testing or unit tests to prove that the change does indeed fix the memmap issue, etc.

I added some docs and tests however testing that memmap is actually working is not so easy (it's not tested currently, some tests are testing memmap=True but without checking that the data is not loaded).

(Also for the record update the PR title and description to be more like the PR that solves the problem instead of an issue reporting it.)

Good point, done.

saimn · 2022-04-22T21:03:40Z

@taldcroft @mhvk - Can you have another look? I think I addressed the various points.

mhvk

Looks good! Only a few textual comments (plus a nitpick)

mhvk · 2022-04-22T21:29:39Z

+    mask_invalid : bool, optional
+        By default the code masks NaNs in float columns and empty strings in
+        string columns. Set this parameter to `False` to avoid the performance
+        penalty of doing this masking step. It's also needing when using


Maybe replace last sentence by "The flag is ignored when using memmap=True..."

mhvk · 2022-04-22T21:30:32Z


    else:

+        if memmap and mask_invalid:


Nitpick: Can just be if memmap:

mhvk · 2022-04-22T21:32:22Z

@@ -0,0 +1,3 @@
+Add a ``mask_invalid`` option to ``Table.read`` to deactivate the masking of


Maybe "... to allow deactivating ..." (since by default the option activates it)

saimn · 2022-04-22T21:45:49Z

@mhvk - Thanks, done.

taldcroft

Looks good to me now.

pllim · 2022-04-23T16:39:20Z

@meeseeksdev backport to v5.0.x

pllim · 2022-04-23T16:40:00Z

@saimn , if you really wants this backported, you have to manually do it. If not, please change the milestone. Thanks!

…option to deactivate the masking of invalid values

Backport PR #12544 on branch v5.0.x (Fix use of memmap with Table.read and add option to deactivate the masking of invalid values)

Pin 3a0cd2d8cd7b459cdc1e1b97a14f3040ccc1fffc and its regression selectors for reproducible runs. Source: astropy/astropy#12544

saimn added io.fits unified-io labels Nov 30, 2021

github-actions Bot removed the unified-io label Nov 30, 2021

pllim added the Bug label Nov 30, 2021

pllim added this to the v5.0.1 milestone Nov 30, 2021

pllim added the backport-v5.0.x label Nov 30, 2021

astrofrog modified the milestones: v5.0.1, v5.0.2 Jan 27, 2022

pllim modified the milestones: v5.0.2, v5.0.3 Mar 10, 2022

saimn mentioned this pull request Mar 23, 2022

Can Table masking be turned off? #12981

Closed

saimn force-pushed the table-memmap branch from 2647c33 to 46e3ef0 Compare March 24, 2022 21:04

saimn force-pushed the table-memmap branch from 46e3ef0 to 64ba9ec Compare March 24, 2022 21:10

pllim modified the milestones: v5.0.3, v5.0.4, v5.0.5 Mar 25, 2022

saimn force-pushed the table-memmap branch from 64ba9ec to c4fd2db Compare April 12, 2022 18:45

saimn requested review from mhvk and taldcroft April 20, 2022 20:17

taldcroft reviewed Apr 20, 2022

View reviewed changes

Comment thread docs/changes/io.fits/12544.bugfix.rst Resolved

taldcroft reviewed Apr 20, 2022

View reviewed changes

saimn and others added 3 commits April 21, 2022 20:10

Add option to avoid loading data when using memmap

2134c6c

Add changelog entry

f739ed8

Add test and docs

a62aa49

saimn force-pushed the table-memmap branch from c4fd2db to a62aa49 Compare April 21, 2022 19:45

saimn added 2 commits April 21, 2022 22:01

Add another test for strings

3a4eea8

Using memmap deactivates the masking

89cdbdd

saimn changed the title ~~Table.read with memmap no more works~~ Fix use of memmap with Table.read and add option to deactivate the masking of invalid values Apr 21, 2022

mhvk reviewed Apr 22, 2022

View reviewed changes

Address review comments

844749f

taldcroft approved these changes Apr 23, 2022

View reviewed changes

taldcroft merged commit 94e8e5b into astropy:main Apr 23, 2022

This comment was marked as resolved.

Sign in to view

lumberbot-app Bot added the Still Needs Manual Backport label Apr 23, 2022

saimn deleted the table-memmap branch April 23, 2022 21:12

saimn pushed a commit to saimn/astropy that referenced this pull request Apr 27, 2022

Backport PR astropy#12544: Fix use of memmap with Table.read and add …

10877e1

…option to deactivate the masking of invalid values

pllim mentioned this pull request Apr 28, 2022

Backport PR #12544 on branch v5.0.x (Fix use of memmap with Table.read and add option to deactivate the masking of invalid values) #13186

Merged

10 tasks

saimn pushed a commit to saimn/astropy that referenced this pull request Apr 28, 2022

Backport PR astropy#12544: Fix use of memmap with Table.read and add …

62abffd

…option to deactivate the masking of invalid values

pllim added a commit that referenced this pull request Apr 28, 2022

Merge pull request #13186 from saimn/auto-backport-of-pr-12544-on-v5.0.x

06df150

Backport PR #12544 on branch v5.0.x (Fix use of memmap with Table.read and add option to deactivate the masking of invalid values)

pllim removed the Still Needs Manual Backport label Apr 28, 2022

weaverba137 mentioned this pull request May 23, 2022

Tmplt subtype desihub/prospect#76

Merged

saimn mentioned this pull request Mar 23, 2023

Broken FITS table round-trip with any nan in a float column. #14558

Open

		@@ -0,0 +1,3 @@
		Add a ``mask_invalid`` option to ``Table.read`` to deactivate the masking of

Conversation

saimn commented Nov 30, 2021 • edited by taldcroft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist for package maintainer(s)

Uh oh!

pllim commented Nov 30, 2021

Uh oh!

saimn commented Nov 30, 2021

Uh oh!

mhvk commented Nov 30, 2021

Uh oh!

saimn commented Dec 2, 2021

Uh oh!

mhvk commented Dec 2, 2021

Uh oh!

saimn commented Mar 24, 2022

Uh oh!

saimn commented Apr 12, 2022

Uh oh!

taldcroft commented Apr 20, 2022

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

saimn commented Apr 21, 2022

Uh oh!

saimn commented Apr 22, 2022

Uh oh!

mhvk 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!

saimn commented Apr 22, 2022

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Apr 23, 2022

Uh oh!

This comment was marked as resolved.

pllim commented Apr 23, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saimn commented Nov 30, 2021 •

edited by taldcroft

Loading