Skip to content

Conversation

@taldcroft
Copy link
Member

Closes #3809.

I still need to add tests and docs, but I just wanted to get this on the radar.

Copy link
Member Author

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.

@barentsen
Copy link
Contributor

@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.

@taldcroft
Copy link
Member Author

Good idea @barentsen, will do.

@taldcroft
Copy link
Member Author

This is impacted by #3915 to add table indexing since replace_column() updates the table in place. Probably better to wait for #3915 to be merged.

cc: @mdmueller

@embray
Copy link
Member

embray commented Aug 31, 2015

Makes sense 👍

@taldcroft
Copy link
Member Author

Added tests and docs, and rebased off of #3915.

@taldcroft
Copy link
Member Author

(Just look at the last 3 commits).

@taldcroft
Copy link
Member Author

To do:

  • - Changelog
  • - Add an exception if trying to replace an index column. This is the short term behavior for this release, then for 1.2 will do the right thing.

(I need to run right now, hence the outstanding actions)

@taldcroft
Copy link
Member Author

Ready for final review @mhvk or @astrofrog

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@mhvk
Copy link
Contributor

mhvk commented Sep 30, 2015

@taldcroft - the build sphinx failure seems to be real:

home/travis/build/astropy/astropy/docs/api/astropy.table.Table.rst:1: WARNING: py:obj reference target not found: name

/home/travis/build/astropy/astropy/docs/api/astropy.table.Table.rst:1: WARNING: py:obj reference target not found: col

table/references.txt:4: WARNING: py:obj reference target not found: name

table/references.txt:4: WARNING: py:obj reference target not found: col

@mhvk
Copy link
Contributor

mhvk commented Oct 1, 2015

@taldcroft - OK, did a final review, and all looks OK, so merging.

mhvk added a commit that referenced this pull request Oct 1, 2015
@mhvk mhvk merged commit ab92a2c into astropy:master Oct 1, 2015
@mhvk
Copy link
Contributor

mhvk commented Oct 1, 2015

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 io.votable (where else...). @embray, @astrofrog, @taldcroft - any ideas? Indeed, what should one even do? Revert?

See https://travis-ci.org/astropy/astropy/jobs/83152048

@embray
Copy link
Member

embray commented Oct 1, 2015

Strange. I can't find a damn thing even referencing what "exited with 245" could mean.

@taldcroft
Copy link
Member Author

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):

astropy/io/votable/tests/util_test.py ...........
astropy/io/votable/tests/vo_test.py ....................................................................................................................
travis_time:end:0927efa0:start=1443722079100632298,finish=1443722359697358939,duration=280596726641
�[0K
�[31;1mThe command "$MAIN_CMD $SETUP_CMD" exited with 245.�[0m

Done. Your build exited with 1.

Maybe 3rd time's the charm?

@mhvk
Copy link
Contributor

mhvk commented Oct 1, 2015

I agree it makes no sense. I restarted the job again...

@embray
Copy link
Member

embray commented Oct 1, 2015

Seems to have worked that time. Guess it really was just a temporary hiccup.

@mhvk
Copy link
Contributor

mhvk commented Oct 1, 2015

OK, thanks for checking. It does seem these types of random errors often happen in votable, or perhaps more relevantly in tests that use external data...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants