DM-39828: move Flag/bool column access from ColumnView to Catalog #695
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's nothing Python-specific about wanting to get a double array in radians from an Angle column.
Nothing here is templated, so we don't need to invoke them N times with different template parameters.
This reduces usage of the ColumnView Python bindings, which will be useful for deprecating some of that as per RFC-816. It also fills in previously-missing support for accessing Angle and Array columns from non-contiguous catalogs.
Calling 'extract' on a Catalog previously delegated to its cached ColumnView object via __getattr__ forwarding, but column-array access on Catalog itself is now more powerful than what ColumnView provides.
There's no special handling of covariance matrices here.
ColumnView now only provides views, with access to column arrays that can only be copies provided by Catalog's own pybind11 bindings.
We were getting away without these because we happened to be including them in the right source files, but this was always fragile.
| template <typename PyClass> | ||
| static void declareBaseColumnViewFlagOverloads(PyClass &cls) { | ||
| cls.def("_basicget", | ||
| [](BaseColumnView &self, Key<Flag> const &key) -> ndarray::Array<bool const, 1, 1> const { | ||
| PyErr_WarnEx( | ||
| PyExc_FutureWarning, | ||
| "Flag/bool access via ColumnView objects is deprecated in favor of more compete support " |
There was a problem hiding this comment.
Do you mean "a more complete support" here?
| fields are silently ignored. | ||
| String fields are silently ignored. Support for `Flag` columns is | ||
| deprecated; at present they are copied into full boolean arrays, but | ||
| after v26 they will be silently ignored as well. |
There was a problem hiding this comment.
Should there be a ticket number that will silently ignore the Flag columns?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The overall goal here is to deprecate some duplication: ColumnView and Catalog both implement access to Flag/bool columns in different ways, and it's cleanest to just move it all to Catalog, since there's no way to actually view Flag/bool column when they're stored as packed bits.