Skip to content

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

Merged
eteq merged 4 commits into
astropy:masterfrom
taldcroft:table-replace-warnings
Dec 12, 2016
Merged

eteq merged 4 commits into
astropy:masterfrom
taldcroft:table-replace-warnings

Conversation

@taldcroft

@taldcroft taldcroft commented Dec 11, 2016 •

Copy link
Copy Markdown
Member

This adds two new table configuration variables which help users with potential issues related to changing the default table setitem behavior for replacement:

  • table.conf.replace_warnings: help to identify possible problems by raising warnings in certain situations.
  • table.conf.replace_inplace: if set to True then revert to the previous "always inplace" behavior for now, with deprecation and then removal of this option in subsequent releases.

Todo:

  • Tests
  • Docs

cc: @cdeil @mhvk @astrofrog @eteq

@taldcroft taldcroft added Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed table labels Dec 11, 2016
@taldcroft taldcroft added this to the v1.3.0 milestone Dec 11, 2016
@cdeil

cdeil commented Dec 12, 2016

Copy link
Copy Markdown
Member

@taldcroft - Thanks!

I'm confused. In the description above you write:

Revert to the previous "always inplace" behavior for now, with deprecation and then removal of this option in subsequent releases.

In docs/table/modify_table.rst you write:

Starting with astropy 1.3 the operation shown above does a complete replacement of the column object.

So is the new default now the same as before, or is it replace for 1.3?


Otherwise, I think the new extended description of the change is great. Thank you for writing it up!


There's one thing that always drives me mad: it's pretty hard to figure out how to get a traceback that shows where warnings come from. Giving that info how to get a filename / line number in the "Finding the source of problems" would be helpful IMO. (could also be a more general part of the docs and then link to it).

@taldcroft

Copy link
Copy Markdown
Member Author

@cdeil - setting the new configuration option table.conf.replace_inplace = True will revert to the pre-1.3 behavior (in-place always), however the default is False and so will have the new (replace-by-default) behavior.

@cdeil

cdeil commented Dec 12, 2016

Copy link
Copy Markdown
Member

OK, thanks for clarifying it for me!

@taldcroft

Copy link
Copy Markdown
Member Author

@cdeil - I updated the description to make it more clear.

@taldcroft

Copy link
Copy Markdown
Member Author

FYI I will be on travel and mostly offline from Monday 7pm EST to Friday 9pm EST.

@mhvk

mhvk commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

@taldcroft - I think this all looks good (I'm not completely sure it is necessary, but certainly good). One worry is whether by default giving warnings on replacing a slice is going to give a lot of false positives. But I guess we will find out. Anyway, no objection to merging.

@taldcroft

Copy link
Copy Markdown
Member Author

@mhvk - the halotools regression (astropy/halotools#637) was a little tricky to diagnose even after we knew that it was due to #5556 (which was possible because @aphearin has astropy-dev connections). So I would be very worried about random person out there getting totally frustrated that their code is failing in an obscure way.

Hopefully not too many false positives, but people can always disable the warnings.

@mhvk

mhvk commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

OK, that was definitely a tricky one. 👍 on the slice warnings. And, on a bit more thought about reversion, continued 👍 on the change to replacing columns.

@eteq

eteq commented Dec 12, 2016

Copy link
Copy Markdown
Member

Alright, I'm going to merge this and backport to 1.3 so it gets into RC1. But I think we should keep an open mind about possibly changing the replace_inplace default to True for 1.3, and then switch to False in 2.0, depending primarily on whether or not we get complaints during the RC.

@eteq
eteq merged commit 63d35e7 into astropy:master Dec 12, 2016
eteq added a commit that referenced this pull request Dec 13, 2016
Add methods to help with potential problems from #5556
@eteq

eteq commented Dec 13, 2016

Copy link
Copy Markdown
Member

backported to v1.3.x in 7b69bad

@taldcroft
taldcroft deleted the table-replace-warnings branch October 1, 2019 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed Ready-for-final-review table

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants