Conversation
fd3430a to
487b915
Compare
|
Rebased down to 3 commits representing the two major changes from @cherti plus my updates. If this passes we can target for 0.4.3 which I hope to release this week (along with maybe v1.0a1?) |
|
Note: This also requires a changelog entry before it is merged. |
astropy/table/column.py
Outdated
There was a problem hiding this comment.
I would vote to just do the max_lines=1 testing even for the object case to avoid the possibility of serious performance impact for big arrays. Also, who knows, maybe there is a subset of rows for which the format is fine and user intends to select that subset prior to outputting. In that case the format string validation would be premature.
There was a problem hiding this comment.
So just get rid of this branch entirely, and call self.pformat(max_lines=1) in all cases?
There was a problem hiding this comment.
That would, however, insist that the format has to match the first line of the column. If the case is considered that people may choose the lines of the column they want to print, this shouldn't be done, I think, for columns of type numpy.object etc.
Considering that the format shall match all entries, checking the first line could be a performance-optimized guess of consistency, though.
|
I don't think I'll have time in the next week to clean this up and finish it; in any case maybe this should be put off a bit longer as it does implement a change in behavior (albeit a desirable one, I think) that tables with invalid column formats will refuse to be instantiated (even if those columns are never displayed). |
|
@embray - good plan, I was also thinking about the issue you just raised. |
|
|
Column implementation in master. This also made a few changes to methods like __array_finalize__ and __setstate__ to not go through the setters for name, unit, and format--these are copying/recreating Columns from existing objects that should presumably already have valid values for those attributes. This also addresses my comment on the original PR here: astropy#3055 (comment) Finally, in order for this to pass testing several existing tests needed to be updated, now that we no longer allow a Column with an invalid format to be created in the first place (a change I support).
487b915 to
ecf0570
Compare
|
Updated this to work against the current master. However, I have a question for @taldcroft: I didn't follow most of the Presumably if a column gets its format from its |
|
@embray - astropy/astropy/utils/data_info.py Line 217 in c1030c3 In reviewing this again there are a couple of things that still need to be fixed up:
|
|
Thanks for the clarification--upon testing myself it seemed to work fine whether I went through |
|
Oh yeah, and so far I have pretty much stuck with the rule of grandfathering |
|
Got it--I think so far this PR sticks with that. |
…dded a docstring for Column.format
|
Conflicts, incomplete (as I understand from the discussions), and unlikely to be completed. Original issue #2868 is still open. |
…dded a docstring for Column.format
This finishes up @cherti's original PR at #3055 by rebasing on master and updating to work on master. I also fixed a few bugs and reformatted the code a bit. After I submit this PR I'm probably going to rebase again to collapse down some of @cherti's original commits, but will still make sure they get credit.