One important thing to note is that this PR is a pre-cursor to other Table optimizations. Right now a lot of table updates (e.g. replacing a column) are slow and/or awkward because they are forced to go through what amounts to full table initialization. This is driven by the requirement to potentially upgrade a table to masked and "upgrade" every other column in the table accordingly.

So with this PR something like replace_column would really be as simple as updating the Table.columns dictionary (#8702). Fast and simple, yay. Part of that would be factoring out much of _init_from_list into a separate method akin to _convert_col_for_table that converts column-like thingy into an acceptable table column object.

I'm in favour about the change in general, and in particular like that normal Table and QTable will both be able to have masked columns mixed in.

One question about the API: as we are changing it anyway, would it be an idea to introduce a new MaskedTable subclass? It would behave a bit like QTable, in that it would change some columns automatically to MaskedColumn (instead of to Quantity).

The main advantage would be that the table code can be much simpler. But it might also help in making the masked property something a bit more logical (like "are there any masked columns" or, if we are willing to break API that far, "are there any masked values?") - when I started reviewing this PR I was quite confused seeing tests that had table.masked is False even though there were masked columns with masked elements in it.

In terms of implementation, initially it would need a Table.__new__ method that checks for masked and passes on to MaskedTable if set (perhaps with a deprecation warning).

Indeed I thought about MaskedTable, and it is a reasonable option to discuss. This would immediately solve the table reading issue I mentioned. However, at the end I suspect we are better off not going that route. This bifurcation would extend to other Table subclasses (starting with QTable) and perhaps lead to more complexity instead of simplicity.

One important thing to remember is that the masked attribute has never been about the contents of the table, but rather defining how to handle adding columns. So this PR does not change that fundamental meaning, and I would argue that is a good thing. It does change the behavior associated with masked=False.

The meaning of masked=True for a Table has not changed at all. It means always add MaskedColumn except for mixins, and for mixins add a mask attribute (as a FalseArray instance) if it doesn't already exist.

Previously, masked=False meant to not add a MaskedColumn, but if that was requested then convert to masked=True and change all existing columns. Now it means add the column using the "most appropriate" class and do not touch other columns.

Definitely the name masked is not the most helpful, compared to say require_masked_columns, but it isn't clear if changing the attribute name at this point is worth it. In the end what I imagine is a user migration toward never using masked=True.

I do think something like a has_masked_columns attribute would be helpful. The definition of this is slightly ambiguous though in terms of whether that means columns that can be masked or columns that have masked elements. The latter is generally more useful, and for mixins like Time you don't know without looking, but there is a question of whether MaskedColumn counts for has_masked_columns if none of the elements are actually masked.

@taldcroft - I see your point about masked - and agree we should change the meaning as little as possible.

This also means the question of whether to use a subclass or not is independent, and at some level an implementation detail: unless the user goes out of their way to check what the actual class of the table instance is, the behaviour of the instance will be as defined by the masked attribute. Which in turn means the decision whether or not to do it should be mostly whether it makes things a lot easier.

I do like the subclass by its analogy with QTable, though, and think that perhaps a more general Table.__new__ with masked and always_apply_units (or something better) and arguments that tell what kind of table behaviour you get.

Or perhaps whether we can push the "agnostic" model a bit further, and also let Quantity just remain itself in a regular Table by default. If so, that would push the question of whether something should be a Quantity or Column to the readers.

Anyway, all that somewhat orthogonal to the current PR, which I think is a good idea regardless of how it is implemented.

I think that's a reasonable API change. In that past, I've found masked columns to be cumbersome in some cases, because they forced the entire table into masked-mode. I don't remember if it was a speed-issue or if some downstream code could not handle the masks, but in any case, having only columns masked that need to be masked is a good idea.

About explicitly round-tripping the table masked attribute, I started down this path and got a partly working solution, but realized it was problematic. Doing this requires updating (hacking) the serialization code to add that specific attribute into the serialized data via a special key in the table meta. It is really not nice.

After doing this I also had the realization that existing table files from masked tables would not have this special meta in them anyway, so they will be read as unmasked, while tables written with astropy >= 4.0 would have that there. So it's just a mess, and the only answer is to accept that if you want a table that is read from file to be masked, then that needs to be done as an explicit step outside of the read process.

All this actually makes astropy I/O more consistent and explicit. Before it was using implicit rules to set masked, and now that will be explicitly handled by the user.

@taldcroft - makes sense; I agree with moving towards a state where masked just becomes irrelevant.

Codecov Report

Merging #8789 into master will decrease coverage by <.01%.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master    #8789      +/-   ##
==========================================
- Coverage   86.72%   86.72%   -0.01%     
==========================================
  Files         405      404       -1     
  Lines       59994    59972      -22     
  Branches     1100     1100              
==========================================
- Hits        52032    52012      -20     
+ Misses       7321     7319       -2     
  Partials      641      641

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 f9d4d95...d856ad7. Read the comment docs.

This is ready for final review.

I think the HTML build is something that's actually fixed on master, so it should go away if you rebase.

@taldcroft - while addressing the review, could you also rebase to get rid of the unrelated CI failure?

Passing all tests now, comments addressed (I think).

At least the memory aspect of FalseArray is easily addressed: it is readonly, so can just be a single element, np.broadcast_to(False, col.shape).

Anyway, for another time!

@astrofrog - I guess I cannot merge without dismissing your request for changes - do you agree the change in wording is good? If so, just go ahead and merge.

I'm certain that the review has been adequately addressed, and thus go ahead and merge, so other PRs based on are not being held up.

Thank you @taldcroft!