Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DISC: deprecate notebook parameter in to_html() #23820

Closed
simonjayhawkins opened this issue Nov 20, 2018 · 8 comments
Closed

DISC: deprecate notebook parameter in to_html() #23820

simonjayhawkins opened this issue Nov 20, 2018 · 8 comments
Labels
Deprecate Functionality to remove in pandas IO HTML read_html, to_html, Styler.apply, Styler.applymap Needs Discussion Requires discussion from core team before further action

Comments

@simonjayhawkins
Copy link
Member

should to_html(notebook=True) be part of the public facing API or called just from repr_html()?

@WillAyd
Copy link
Member

WillAyd commented Nov 21, 2018

This is already documented in the API in development:

http://pandas-docs.github.io/pandas-docs-travis/generated/pandas.DataFrame.to_html.html?highlight=to_html

I'm not clear by the issue if you are asking whether it should be documented or deprecated altogether, though I would be -1 on deprecation (clarification would be helpful).

@WillAyd WillAyd added IO HTML read_html, to_html, Styler.apply, Styler.applymap Code Style Code style, linting, code_checks Needs Info Clarification about behavior needed to assess issue labels Nov 21, 2018
@simonjayhawkins
Copy link
Member Author

i'm not au fait with the deprecation process, hence opened an issue to start a discussion.

i assume that if deprecation is agreed then a deprecation warning is issued in the next release and the change is not made for a couple of releases. I hadn't thought about just removing it from the documentation... that could be a good start.

personally, I don't think it's necessary from a user perspective (i.e from df.to_html()) and hence removing it would simplify the API.

from a code perspective, i'm looking to make HTMLFormatter a subclass of DataFrameFormatter instead of passing the DataFrameFormatter object to the HTMLFormatter on instantiation.

This could naturally evolve in time with enhancements and feature requests into separate subclasses of HTMLFormatter.. say NotebookFormatter and ToHTMLFormatter. Having the notebook parameter in to_html() would not prevent this, just make it less straightforward.

@WillAyd
Copy link
Member

WillAyd commented Nov 22, 2018

I'll admit I'm not terribly familiar with this parameter nor its usage. If you have a code cleanup in mind though I suppose a PR is the best way to go about it unless one of the other core devs objects to the possibility of deprecation here.

To answer your question about deprecation, yes we typically issue a FutureWarning for deprecated parameters and do the actual removal a few releases later. You can find other examples of these in closed PRs using the Deprecate label

@WillAyd WillAyd added Deprecate Functionality to remove in pandas Clean and removed Needs Info Clarification about behavior needed to assess issue labels Nov 22, 2018
@simonjayhawkins
Copy link
Member Author

If you have a code cleanup in mind though I suppose a PR is the best way to go about it unless one of the other core devs objects to the possibility of deprecation here.

i'll aim to put a draft PR together for the deprecation shortly.

I'd like to have a base class for to_html that is shared with NotebookFormatter rather than the current situation where HTMLFormatter is the base class of NotebookFormatter.

Having notebook parameter in to_html() method effectively means that all the functionality of to_html has to be available with notebook=True option and limits divergence in functionality.

@jorisvandenbossche
Copy link
Member

@simonjayhawkins if I am correct, what the notebook argument does is adding some styling so it renders nicely in a notebook?
This could in principle be useful for users as well? (eg to create html to then paste into a notebook markdown cell)

@simonjayhawkins
Copy link
Member Author

@simonjayhawkins if I am correct, what the notebook argument does is adding some styling so it renders nicely in a notebook?

It just adds a <div> and some style.

       # We use the "scoped" attribute here so that the desired
       # style properties for the data frame are not then applied
       # throughout the entire notebook.

This could in principle be useful for users as well? (eg to create html to then paste into a notebook markdown cell)

Yes. to_markdown() would be better.

a couple of issues.

  1. At the moment there is not a clear separation between the display in the notebook (repr) and HTML as an output format (io). Display options bleed into the to_html() as an output format.
  2. Many permutations of to_html() parameters are not tested with notebook=True. Only parameters relevant to the notebook _repr_html_ are tested. IMO the implementation of the notebook argument of to_html() as an output format is incomplete.

Both these issues could still be addressed without deprecating the notebook argument.

The first was effectively started with this PR. It is necessary to dispatch to DataFrameFormatter directly instead of calling to_html.

The second requires additional tests and a clean implementation of the application of display options.

@jorisvandenbossche
Copy link
Member

I personally think this functionality might be useful for people that want to reuse our html machinery for displaying their object. But they could maybe also use a HTMLFormatter directly?

At the moment there is not a clear separation between the display in the notebook (repr) and HTML as an output format (io). Display options bleed into the to_html() as an output format.

That's true for other options as well, no? (such as max_rows/max_cols)

@simonjayhawkins
Copy link
Member Author

But they could maybe also use a HTMLFormatter directly?

or just use the internal _repr_html_ method.

That's true for other options as well, no? (such as max_rows/max_cols)

max_rows, max_cols and show_dimensions are not an issue.

pandas/pandas/core/frame.py

Lines 648 to 654 in dcba7a5

if get_option("display.notebook_repr_html"):
max_rows = get_option("display.max_rows")
max_cols = get_option("display.max_columns")
show_dimensions = get_option("display.show_dimensions")
return self.to_html(max_rows=max_rows, max_cols=max_cols,
show_dimensions=show_dimensions, notebook=True)

column_space, float_format, and precision are handled 'centrally' by format_array

if space is None:
space = get_option("display.column_space")
if float_format is None:
float_format = get_option("display.float_format")
if digits is None:
digits = get_option("display.precision")

#25977 does the same for max_colwidth and subsequent PRs would be required for the other display options.

So this can still being addressed without deprecating the notebook argument.

I personally think this functionality might be useful for people that want to reuse our html machinery for displaying their object.

If you are against deprecating the notebook parameter and the discussion continues beyond the 0.25 release, i guess it won't happen. I opened this issue before the 0.24 release, but didn't get much of a response.

IMO it's unnecessary, but not a big deal if we don't. I guess the only way to know if users rely on it is with a Deprecation Warning because I don't know and can't argue that point, so feel free to close this if you disagree.

@simonjayhawkins simonjayhawkins removed the Code Style Code style, linting, code_checks label Jul 7, 2019
@simonjayhawkins simonjayhawkins added Needs Discussion Requires discussion from core team before further action and removed Clean labels Oct 7, 2019
@simonjayhawkins simonjayhawkins added this to the No action milestone Dec 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas IO HTML read_html, to_html, Styler.apply, Styler.applymap Needs Discussion Requires discussion from core team before further action
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants