Check results are now reported in the status checks at the bottom of this page.

It would be more generic to handle this case in Table.replace_column instead of raising an error. Why not dropping the index there, maybe with a keyword or warning ?

@taldcroft - do you have any thoughts on this?

@saimn - on further thoughts, I guess my thinking is that Table has already made a choice to raise a clear exception in this use case of replace_column, and this PR fixes a bug in the FITS code that uses that. Changing the behavior in Table would actually be an API change. So I personally think we should just treat this as a bug fix and not modify Table.

I guess we could also add a keyword argument to control this but again this would be a change in API and I'd like to get this fix in v3.1.

This looks fine as a bug fix. Ideally the right answer is to serialize the index so it round-trips, which would make generating the index more useful since it would persist. In theory the machinery is mostly there, but it probably would not be trivial in the end.

@astrofrog - I can understand your reasoning, but for me it is a Table thing, not FITS or Time. Looking back in time, #4090 (comment) shows - I think - that there is no real reason for raising an error in this case.
I don't know what would the good solution when replacing a column with an index (recreating all the indexes using this column may be tricky), but a simple solution could be to add a drop_index keyword, which would not change the default behavior.
And maybe another option would be to remove the use of replace_colum, I'm not sure if it is necessary here ? At least replacing it with newtable[col.info.name] = jd12 does not cause a failure in the unit tests.

@taldcroft - the serialization is not the issue (it seems that indexes are dropped when writing tables to fits), the crash happens when using replace_column here:

Codecov Report

Merging #8077 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8077      +/-   ##
==========================================
- Coverage   86.96%   86.92%   -0.04%     
==========================================
  Files         408      383      -25     
  Lines       61428    57826    -3602     
  Branches     1096     1056      -40     
==========================================
- Hits        53422    50267    -3155     
+ Misses       7383     6945     -438     
+ Partials      623      614       -9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 253ea74...af611a3. Read the comment docs.

Ok let's delay this for now, I don't have time to work on it more for 3.1

Hi humans 👋 - this pull request hasn't had any new commits for approximately 7 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

I re-opened this as I think that this approach is OK and fixes a real issue.

The only outstanding problem is the comment above that the current code actually corrupts the original table by removing indices on newtable. That's basically a bug that I plan to fix in #9009.

@astrofrog - could you please rebase and address the comment to make it ready to be merged?

@bsipocz - I think you just missed that this would be best done by waiting on a fix for #9009, at which point the table copy (with copy=False) will do the right thing.

However, there definitely should be a test that this operation does not corrupt the original table. That will fail right now, but will start working once #9009 is fixed.

Oh, indeed. Then I add the keep-open here, as either that or a rebase that makes sure this doesn't get closed again when the bot runs next time.

@astrofrog et al. - I think should be good now, please review. The crux lines of code depend on table improvements in 4.0-dev, so I don't recommend trying to backport.

@taldcroft - There are no more backports except critical fixes like the one we need for iers :)

@taldcroft - #9009 still seems to be relevant, is that not an issue for this any more?

#9009 is still relevant because it will make it possible to improve the implementation here to avoid making any copies. Right now it does copy any mixin columns that have indices. So I put in a breadcrumb in #9009 to come back here and have a look.