Change Table behavior when setting column: default to replace by taldcroft · Pull Request #5556 · astropy/astropy

taldcroft · 2016-12-05T14:27:56Z

This PR has a very simple code change so that when setting an existing Table column the code first tries to call replace_column, and only if that fails it tries the old behavior of doing an inplace update. So in the past it was always an inplace update:

t['a'] = val  # equivalent to t['a'][:] = val

With this PR:

t['a'] = val  # equivalent to t.replace_column('a', val)

It might happen that replace_column will fail (e.g. if val = 1) in which case the inplace update will happen.

On the whole I believe that this new behavior is generally more intuitive. It matches how pandas behaves, and matches the new (post astropy 1.0) basic concept that a table is a collection of independent columns. The inplace update was previously forced by using a numpy structured array underneath to hold the table data.

I have been increasingly bothered by the inplace update behavior and the problems it creates (e.g. #2630). I previously have argued that changing this behavior would introduce unexpected behaviors (#3809). In reviewing my arguments I think those problems are definitely corner cases and not likely to impact many people. In short I have changed my mind!

One thing to be aware of is something like this:

>>> t = Table([[1, 2]], names=['a'])
>>> ta = t['a']
>>> t['a'] = [3, 4]
>>> t['a'] is ta  # Completely new column
False

I'm not sure how surprising or problematic this would be. One small variant of this PR is to require that the right-hand side be either a Column subclass or a mixin column. That would eliminate the above, but at the cost of less consistent / simple behavior. For instance if t['a'] were a Time mixin and you did t['a'] = [3, 4] then it fails because you cannot to an inplace assignment to a Time object.

I've been thinking about this change for some time, but seeing #2630 and the complications that come up made me seriously consider this API change.

@cdeil @mhvk @astrofrog

… = val

mhvk · 2016-12-05T17:43:17Z

I like the change in principle for normal columns, but am a bit worried about how it affects setting the mask: if you do table.mask['b'] = something, it will now not affect the mask of table at all, without warning (indeed, this is, I think, the reason for the failing io.fits test).

Need to think a bit about what behaviour would be best...

taldcroft · 2016-12-05T18:54:46Z

@mhvk - yes, so this is something to consider. In retrospect introducing the idiom of t.mask['a'] = val was a mistake. The "proper" way to say this is t['a'].mask = val.

Two ways to handle this which would fix the test failure and eliminate the need for the patch in the PR to the test function related to masking:

Implement the idea I mentioned where the right hand side must be a Column-like object.
Make a InplaceTable class which uses the old-style inplace update no matter what.
With this option, there would a question of whether to expose that class to users who want that behavior. One might also consider a table attribute inplace_setitem for the same. But honestly it is better / easier to just fix code, namely t['a'] = val => t['a'][:] = val. That is an idiom that is well known and familiar even to users of Python lists for inplace updates.

Upshot - for the mask issue we might want to consider a private _inplace_setitemattribute that makes it so that t.mask['a'] = val works as expected. That will remove code breakage that would otherwise be obscure / difficult to find because it involves a broken reference in the mask attribute (which is a dynamically created table).

mhvk · 2016-12-05T19:17:53Z

How about we do use a private attribute such that mask is updated in place, but at the same time deprecate the use of table.mask[column]? I agree that table[column].mask is more logical & better and it seems best just to provide one method.

Along similar lines: is it an idea to deprecate the in-place setting instead of trying it when replace_column fails? As you say, it is clearer to do t['a'][:] = val.

taldcroft · 2016-12-05T19:31:26Z

Along similar lines: is it an idea to deprecate the in-place setting instead of trying it when replace_column fails? As you say, it is clearer to do t['a'][:] = val.

Offhand I'm not sure I would get that pedantic. It's still nice to do stuff like t['a'] = 0 to zero out a column. I would find it a bit annoying to try that and get some exception saying, essentially, "We know exactly what you want to do, but don't write your code that way."

mhvk · 2016-12-05T20:26:37Z

OK, agreed. But deprecating table.mask may still be an idea. Though at some level it is slightly orthogonal to the discussion at hand: for now, I do think we'll need to ensure that setting table.mask['a'] is done in-place.

cdeil · 2016-12-06T07:44:14Z

👍 if this fixes #2630 .

More tests to establish the behaviour of this fundamental table operation would be good, no?
E.g. with the code change, does the test from https://github.com/astropy/astropy/pull/2630/files#diff-e3a41dcbfc9a7ee76131b1c730e3d268R1752 now pass.

Most of the discussion with @mhvk above concerns mask, but I don't see any new test mentioning mask added. Is this because those tests already existed and the new behaviour wrt. masks is the same? Or is the behaviour wrt. masks not established via tests yet?

To be honest, the discussion and suggestions how to handle mask above are a bit over my head. Would probably be good to get one or two more people familar with Table to comment, maybe @astrofrog?

Just one question concerning:

>>> from astropy.table import Table, Column
>>> table = Table()
>>> table['a'] = Column([1, 2, 3], unit='m', description='original')
>>> table['a'] = [4, 5, 6]

Before this PR, this updated the values, but preserved unit and description.
After this PR, the unit and description will be gone for table['a']?
If yes, I'd suggest to make this a little more explicit in the changelog entry (which already has an example in this direction, but doesn't spell it out what exactly the changed behaviour implies for this case).

taldcroft · 2016-12-06T11:57:05Z

@cdeil - thanks for having a look and thinking about this.

Most of the discussion with @mhvk above concerns mask, but I don't see any new test mentioning mask added. Is this because those tests already existed and the new behaviour wrt. masks is the same? Or is the behaviour wrt. masks not established via tests yet?

No new tests were added because there were existing tests of masking that failed. If you look at the first commit dd8ea97 there was a change to an explicit test of masking in test_masked.py in order for tests to pass. After discussions the code patch was changed and the original test was reverted. After the initial commit there was also a test failure in io.fits related to masking (same problem, different test) which now passes.

About the light testing, this change is very simple in substance since it just delegates to replace_column, which is well tested. So the only question is whether the new right-hand-side value will succeed in the call to t.replace_column(colname, new_value). Once that happens we can trust that the new column is there because replace_column works. Upshot: replace is simpler and more reliable than in-place update.

In the Replace block of the new test, the key thing to look at is the final line assert t['a'] is not ta. That ensures that the column was really replaced with a new object. Your test case is a duplicate test, albeit with a more complex object on the right side. I did explicitly check that it passes, but if you would like it in the formal tests I will add it.

taldcroft · 2016-12-06T12:00:57Z

After this PR, the unit and description will be gone for table['a']?

Correct. The column is entirely replaced with whatever value is on the right side. I will update the API change entry to be even more clear.

Agreed that someone like @astrofrog should weigh in. I also thought about sending a separate email to astropy-dev to allow for comment. I did this already, but perhaps under the label astropy 1.3 freeze it got lost.

taldcroft · 2016-12-06T12:08:47Z

OK, I sent email to astropy-dev.

cdeil · 2016-12-06T12:25:57Z

In the Replace block of the new test, the key thing to look at is the final line assert t['a'] is not ta. That ensures that the column was really replaced with a new object. Your test case is a duplicate test, albeit with a more complex object on the right side. I did explicitly check that it passes, but if you would like it in the formal tests I will add it.

Thanks for explaining. I agree there's not need to add the extra test then.

mhvk · 2016-12-06T21:01:01Z

-                self.columns[item][:] = value
+                try:
+                    # See definition of mask property for discussion of next line.
+                    assert not getattr(self, '_setitem_inplace', False)


I've used this structure with an assert statement earlier myself, but was warned that all assert statements are skipped if one runs python with optimize (i.e., after python3 -O, typing assert False does not do anything...). So,

# The below statement is to support self.mask[col] = value. # If that is deprecated, this can be removed. if getattr(self, '_setitem_inplace', False): try: self.replace_column(item, value) return except Exception: pass self.columns[item][:] = value

Ouch, this is good to know. There are a few places (~50) in the code where an assert is used, so you suggest that ideally we should get rid of those?

An assert is fine if it tests something that is expected to be true -- it would seem OK to omit that test when one optimizes -- but not when it is used to decide which logical path to take. So, I guess OK except in try/except clauses.

That said, obviously it is better to do

if <something-unlikely>: raise ShouldNotHappenError('...')

mhvk · 2016-12-06T21:02:53Z

This looks all OK except for the one remaining comment.

@taldcroft

modifies the galaxy_table in-place, as recommended by @taldcroft, in a first attempt to make Halotools compatible with astropy/astropy#5556.

taldcroft · 2016-12-11T12:04:23Z

Based on discussion in astropy/halotools#637, I think we need a follow-on PR to issue a warning when replacing a column that is a slice. This condition would simply be if the column is an ndarray subclass and flags.OWNDATA is False.

I also thought about possible ways to warn if the replacement might be doing something unexpected, with a config var to control this. This is obviously a bit trickier. For instance replacing a column with data of the same type but losing metadata. Or even just issuing a warning with a reference to the calling line any time a replacement is done, for debugging purposes. Any thoughts?

taldcroft · 2016-12-11T15:38:49Z

Dang, I forgot that flags.OWNDATA is basically always False for ndarray subclasses (or at least for Column). So plan B is using the base attribute. Is that reliable @mhvk?

cdeil · 2016-12-11T16:15:45Z

I also thought about possible ways to warn if the replacement might be doing something unexpected, with a config var to control this.

This option sounds to me like it's too complicated.
(pandas has no such config option, right?)

I think it's OK to keep the change for 1.3.

Yes, it's a change in behaviour and it will bite a few people.
But the current behaviour did also bite a few people and the reports were labeled as "bug", right?

@taldcroft - If you're concerned with doing this change in 1.3, maybe postponing it to 2.0, i.e. a major version jump, is an option?

taldcroft · 2016-12-11T16:29:26Z

@cdeil - in fact Pandas does have an option to control warnings for copy/replace issues.
http://pandas.pydata.org/pandas-docs/stable/indexing.html#indexing-view-versus-copy

I think the pandas stuff is a bit overcomplicated, but the #5586 warnings should be manageable.

I'd like to get this for 1.3 if possible!

Add methods to help with potential problems from #5556

@bsipocz

…e is explicitly modified in-place in accord with astropy/astropy#5556. This change has previously been brought as part of astropy#637 by b28fe15. However, as that PR is entirely unrelated to this bugfix, astropy#637 will instead be rebased against this commit for clarity. Merging this PR will also resolve the test failures that occur in astropy#638. @bsipocz and/or @eteq - let me know if you know how to handle the git rewrite in a more elegant way (e.g., rolling back b28fe15), otherwise I'm fine with simply locally rebasing astropy#637, locally resolving the conflict, and then force-pushing astropy#637.

Try replace_column before inplace when setting existing column t['a']…

dd8ea97

… = val

pllim added Affects-release table labels Dec 5, 2016

taldcroft added 2 commits December 5, 2016 17:09

Fix issue with setting Table.mask attribute

8d791d1

Update changelog and docs

75185a0

taldcroft added this to the v1.3.0 milestone Dec 5, 2016

astrofrog approved these changes Dec 6, 2016

View reviewed changes

eteq approved these changes Dec 6, 2016

View reviewed changes

astrofrog added the whatsnew-needed label Dec 6, 2016

mhvk reviewed Dec 6, 2016

View reviewed changes

Change logic to avoid using assert; add clarification to changelog

3a06cab

taldcroft assigned mhvk Dec 7, 2016

taldcroft merged commit 1849373 into astropy:master Dec 7, 2016

taldcroft deleted the table-replace branch December 7, 2016 01:58

cdeil mentioned this pull request Dec 7, 2016

Table column assignment doesn't update column description #2630

Closed

This was referenced Dec 9, 2016

Subhalo-based phase space model for satellites astropy/halotools#637

Merged

Cleanup: Removing code asserts #5580

Merged

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

Add methods to help with potential problems from astropy#5556

079b462

taldcroft mentioned this pull request Dec 12, 2016

Add methods to help with potential problems from #5556 #5586

Merged

2 tasks done

saimn mentioned this pull request Dec 12, 2016

Crash with the new way to replace a column #5595

Closed

eteq added a commit that referenced this pull request Dec 12, 2016

Merge pull request #5586 from taldcroft/table-replace-warnings

63d35e7

Add methods to help with potential problems from #5556

eteq added a commit that referenced this pull request Dec 13, 2016

Merge pull request #5586 from taldcroft/table-replace-warnings

7b69bad

Add methods to help with potential problems from #5556

aphearin mentioned this pull request Dec 18, 2016

Change Table column creation to be compatible with new astropy syntax astropy/halotools#639

Merged

dhomeier pushed a commit to dhomeier/astropy that referenced this pull request May 24, 2017

Add methods to help with potential problems from astropy#5556

3a2c402

bsipocz mentioned this pull request Jul 23, 2018

New feature - Solar System ephemerides from the Minor Planet Center astropy/astroquery#1177

Merged

mkelley mentioned this pull request Jul 24, 2018

Avoid asserts in parameter testing NASA-Planetary-Science/sbpy#49

Closed

Conversation

taldcroft commented Dec 5, 2016

Uh oh!

mhvk commented Dec 5, 2016

Uh oh!

taldcroft commented Dec 5, 2016

Uh oh!

mhvk commented Dec 5, 2016

Uh oh!

taldcroft commented Dec 5, 2016

Uh oh!

mhvk commented Dec 5, 2016

Uh oh!

cdeil commented Dec 6, 2016

Uh oh!

taldcroft commented Dec 6, 2016

Uh oh!

taldcroft commented Dec 6, 2016

Uh oh!

taldcroft commented Dec 6, 2016

Uh oh!

cdeil commented Dec 6, 2016

Uh oh!

mhvk Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

bsipocz Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

mhvk Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

mhvk commented Dec 6, 2016

Uh oh!

taldcroft commented Dec 11, 2016

Uh oh!

taldcroft commented Dec 11, 2016

Uh oh!

cdeil commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taldcroft commented Dec 11, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cdeil commented Dec 11, 2016 •

edited

Loading