BUG/ENH: Styler.format() always inherits from .set_na_rep()
#40060
Conversation
attack68
changed the title
Styler.format() always inherits from .set_na_rep()
| def format( | ||
| self, | ||
| formatter: Optional[ | ||
| Union[Dict[Any, Optional[Union[str, Callable]]], str, Callable] |
There was a problem hiding this comment.
can you type this at the top / in conftest.py (note that we can probably generally type these formatters across IO functions, but that's for another day)
| """ | ||
| Format the text display value of cells. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| formatter : str, callable, dict or None | ||
| If ``formatter`` is None, the default formatter is used. | ||
| Format specification to use for displaying values. If ``None``, the default | ||
| formatter is used. If ``dict``, keys should corresponcd to column names, |
| Representation for missing values. | ||
| If ``na_rep`` is None, no special formatting is applied. | ||
| Representation for missing values. If ``None``, will revert to using | ||
| ``Styler.na_rep`` |
There was a problem hiding this comment.
hmm why are you adding this special function? we don't do this anywhere else
There was a problem hiding this comment.
I think your comment about the fact that nowhere else is there a set_na_rep method that lends itself to the answer here. It is possible for there to be a solution where both functions (that currently exists) operate consistently, as opposed to somewhat inconsistently and inconveniently at present
There was a problem hiding this comment.
I would add that this might not even be interpreted as a change. no special formatting applied might be expected to be that `Styler.na_rep`` is used as default, it is ambiguous. I was just clarifying what the bug fix did here.
| @@ -1294,6 +1307,40 @@ def hide_columns(self, subset) -> Styler: | |||
| self.hidden_columns = self.columns.get_indexer_for(hidden_df.columns) | |||
| return self | |||
|
|
|||
| def _default_formatter(self, x): | |||
There was a problem hiding this comment.
why is this not a module level function?
| return f"{x:.{self.precision}f}" | ||
| return x | ||
|
|
||
| def _maybe_wrap_formatter( |
There was a problem hiding this comment.
why is this moved? what is the point of self.na_rep?
There was a problem hiding this comment.
its moved because as a module level it can't act dynamically and update self.na_rep if it is later changed. At class level it can so this goes to inheritance.
if there are 2 let's deprecate one. we use we need one canoncial way to do things. |
jreback
added
API - Consistency
|
try not to mix bug fixes with api changes in a single pr. keep them as focused as possible. |
|
@jreback I agree somewhat that maybe multiple different |
|
I think the PR above is better both in terms of API and code maintenance. Do close this PR if you agree. |
na_reparg overwrites existing when not given. #40032test_display_format_subset_interaction: why: see below.test_format_bad_na_rep: why:na_repworks if not string in some cases - changed to duck-typing.test_display_format,test_display_set_precision,test_format_subset,test_display_dicttest_display_format_raisesThe issue is that there are currently two competing methods in
Stylerfor missing value represenatation:.set_na_rep('x')and.format(na_rep='y').The two do not interact well together dynamically. This PR brings the
format wrappinginto the class so thatformatcan be updated and inherit values inset_na_repafter it has been called. The image below highlights the impact this PR has and the added test replicates this pattern:Also at the bottom here, we add a
KeyErrorto remove the ambiguity between what happens when a dict is provided with a column Key that is not specifically included within the providedsubsetslice.