-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Add Table.replace_column() method #4090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
astropy/table/table.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to self: improve the description of this example.
|
@taldcroft Thanks for working on this! I think this is a necessary method, which offers a practical solution to #3809. Suggestion: I think it would be good to summarize the raison d'être for this function in the docstring, i.e. to explain the difference between a "column replacement" and a "column assignment" operation (even if just in 2 or 3 sentences). This docstring would be the first place where I'd look to find out about this. |
|
Good idea @barentsen, will do. |
|
This is impacted by #3915 to add table indexing since cc: @mdmueller |
|
Makes sense 👍 |
0a72e93 to
952720b
Compare
|
Added tests and docs, and rebased off of #3915. |
|
(Just look at the last 3 commits). |
|
To do:
(I need to run right now, hence the outstanding actions) |
952720b to
6bb711b
Compare
6bb711b to
fa73222
Compare
|
Ready for final review @mhvk or @astrofrog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand, this does not disallow
t['a'] = [1,2,3]
t['a'] = [1,2,4]correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. This is in TableColumns, so what is now disallowed is t.columns['a'] = [1, 2, 3]. Previously this would not fail but the results would be unpredictable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
|
@taldcroft - the build sphinx failure seems to be real: |
|
@taldcroft - OK, did a final review, and all looks OK, so merging. |
|
Unfortunately, it seems that after merging, the build of this PR continuously fails for python 3.4 (I tried restarting the build twice), getting stuck in |
|
Strange. I can't find a damn thing even referencing what "exited with 245" could mean. |
|
There's really nothing in this PR that should impact votable. FYI the full log ends a little differently than the HTML log (it gets through much of vo_test.py): Maybe 3rd time's the charm? |
|
I agree it makes no sense. I restarted the job again... |
|
Seems to have worked that time. Guess it really was just a temporary hiccup. |
|
OK, thanks for checking. It does seem these types of random errors often happen in |
Closes #3809.
I still need to add tests and docs, but I just wanted to get this on the radar.