Repository navigation
Consider changing the default behavior of Table.pformat() to be independent of the terminal #15828
Description
What is the problem this feature will solve?
Currently the Table.pformat() default is to respect the size of the interactive terminal when generating lines of output representing the formatted table. This matches what would be seen with the corresponding Table.pprint() call (more-or-less).
The problem is that in most cases pformat() is used in a context where an interactive terminal is not relevant or does not exist (in which case 24x80 is assumed). This leads to unexpected results or the need for workarounds. A lot of test code uses pformat_all() as a work-around.
Issue #14010 is a direct result of this unexpected behavior because str(table) also respects the terminal size.
Describe the desired outcome
By default Table.pformat() would be equivalent to Table.pformat_all(), i.e. use max_lines=-1 and max_width=-1. This would make pformat() independent of the terminal and lead to fewer suprises.
Additional context
The big concern is whether this change will cause unexpected code breakage. This would need to wait until 7.0 but if merged into main there would be plenty of time for testing.
Activity
taldcroft commentedon Jan 8, 2024
cc: @mhvk @neutrinoceros
neutrinoceros commentedon Jan 8, 2024
Seems like this would be the least surprising behaviour it could have indeed.
Regarding compatibility concerns, what is not immediately clear to me is whether we could have a deprecation period before the actual change in default behaviour. I mean in principle it's not really complicated to do, but I'm worried that such deprecation warnings could be all over the place and that silencing them would be tedious (the most obvious way we could offer to silence them would be to specify
max_lines=-1, max_width=-1explicitly on all call sites)mhvk commentedon Jan 8, 2024
It would seem required to solve #14010 to ensure that
str(table)usestable.pformat_all(). Isn't the simplest solution then to just do that instead of making.pformatand.pformat_allaliases?taldcroft commentedon Jan 8, 2024
The least invasive way to fix #14010 is just to put the relevant bits of that function into a context manager that sets the default conf variables to -1. So that doesn't touch table at all.
I had not thought of just changing
str(table)to usepformat_all(), but I think that would be a half-way solution that still (potentially) breaks stuff but without fixing everything. By now I have learned to always usepformat_all()in every test, but it is silly when that should be functionally equivalent topformat()andstr(table)andrepr(table). They should all just show the entire table, end of story.I don't know why we chose to go in the other direction a decade ago but it just seems very wrong now.
taldcroft commentedon Jan 8, 2024
And @neutrinoceros you are right that there is no way (that I can think of) to have a deprecation. In theory we could introduce new configuration variables that would make it possible to revert to the old behavior. I suspect it would get ugly though.
mhvk commentedon Jan 8, 2024
Indeed, purely from an API perspective, there should have been only one
.pformatwhich shows everything by default; onlypprintshould really worry about terminal size.Anyway, I'm OK with changing it, though ideally we'd then get rid of
.pformat_all, at least in the documentation. Perhaps aPendingDeprecationWarning(to be changed in aDeprecationWarningonce 6.1 is longer supported)? Or were you thinking of just keep it around forever aspformat_all = pformat?taldcroft commentedon Jan 8, 2024
I would be in favor of deprecating
.pformat_all, that's easy enough. The main question is getting community buy-in (probably emails to astropy an astropy-dev), and then timing. We just missed 6.0, which would mean 1.5 long years to get this in... I'm not really loving the new release policy.Thinking more about this, maybe it would be palatable to the release managers and community to include a change like this in 6.1 if there was a single config option to revert to the old behavior. So it would be potentially breaking but with an easy fix. Then in 7.0 we remove that config option (or being even more cautious we deprecate it 1).
Footnotes
I'm not sure if you can deprecate a config option. ↩
pllim commentedon Jan 8, 2024
Hmm I don't know an easy way of deprecating a config option either (at least not in recent memory). It has aliases but that is for moving it around. Technically it is possible (e.g., detecting it being assigned etc) but I am just not sure if there is easy-to-use machinery in place.
This sounds drastic though, so might be worth scouring GitHub to see if anyone actually uses it, and maybe asking over at astropy-dev.
mhvk commentedon Jan 8, 2024
It's less than a year away, no? At least I thought 7.0 is nominally meant to be end of '24.
neutrinoceros commentedon Jan 9, 2024
for my own sake, I looked up APE 21, and here's the relevant quote
Seems to me that we already have plans for breaking stuff, so 7.0 could happen as soon as December 24 ?
taldcroft commentedon Jan 9, 2024
Yup, I had forgotten that APE-21 puts the major release schedule as early as Dec 24. I was remembering the LTS schedule of once each 3 minor releases (1.5 years). So l guess one year will go by soon enough.
pllim commentedon Oct 10, 2024
It is close to v7.0 feature freeze and I don't see any PR out to close this issue, and since this does not block release, I am removing the milestone. Thanks for your understanding.
neutrinoceros commentedon Oct 10, 2024
So basically our next window to try this is after 7.1 is branched out (assuming we don't plan a 7.2). Is this right ?
pllim commentedon Oct 10, 2024
Hmm... According to revised https://github.com/astropy/astropy-APEs/blob/main/APE2.rst , it would be v8.0? Maybe @astrofrog can confirm since he wrote astropy/astropy-APEs#82
neutrinoceros commentedon Oct 11, 2024
That's what I meant, really: we can only change the default behavior on a branch that's aimed to become a major release, which we won't have in the interval between 7.0's publication and 7.1's branching (supposedly that's synonymous with 8.0's)
taldcroft commentedon Oct 11, 2024
@neutrinoceros - glad you remembered this! A bit late but IMO we should get this in 7.0. From @mhvk --
Agreed that ideally there should be only one
.pformat. What aboutPendingDeprecationWarningfor 7.0 andDeprecationWarningfor 8.0 and then remove in 9.0? That seems pretty conservative.mhvk commentedon Oct 11, 2024
I'm fine with just deprecating now, but obviously also with the more conservative route. I think it is nice to still get it in...
pllim commentedon Oct 11, 2024
Get what in though? I don't see any PR set to close this issue.
mhvk commentedon Oct 11, 2024
I think the PR would be fairly straightforward - this was more about deciding how to approach it...
pllim commentedon Oct 11, 2024
FWIW this close to feature freeze, the less controversial, the better.
neutrinoceros commentedon Oct 12, 2024
Well, @pllim is the one who deserves praise for this :)
I think I should be able to handle this one since it actually seems rather uncontroversial (among devs). I'll still go with Tom's plan to only use a
PendingDeprecationWarningto minimize controversiality (???).neutrinoceros commentedon Oct 15, 2024
The PR is at #17179