-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
ENH: Style blocks #15954
ENH: Style blocks #15954
Conversation
TomAugspurger
commented
Apr 8, 2017
Here's a gist with the new docs: http://nbviewer.jupyter.org/gist/anonymous/db1b30a4ba4085bda612944c6ba9b64e#Extensibility I think right now the template isn't included in the package, need to update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all for the reorg, do we need this in the top-level namespace?
maybe make a sub-dir for the style things at some point (e.g. pandas/formats/style
(but another PR if you think its worth it).
doc/source/whatsnew/v0.20.0.txt
Outdated
@@ -358,6 +358,9 @@ Other Enhancements | |||
- ``DataFrame.to_excel()`` has a new ``freeze_panes`` parameter to turn on Freeze Panes when exporting to Excel (:issue:`15160`) | |||
- ``pd.read_html()`` will parse multiple header rows, creating a multiindex header. (:issue:`13434`). | |||
- HTML table output skips ``colspan`` or ``rowspan`` attribute if equal to 1. (:issue:`15403`) | |||
- ``pd.Styler`` template now has blocks for easier extension (:issue:`15649`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the right one? (maybe use a :class:`....`
?
pandas/__init__.py
Outdated
@@ -56,6 +56,7 @@ | |||
from pandas.util.print_versions import show_versions | |||
from pandas.io.api import * | |||
from pandas.util._tester import test | |||
from pandas.formats.style import Styler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you really want to add this to the top-level? (does this import stuff)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it needs to be importable somewhere 'public', not sure if top level is the right spot, could argue either way. Do we need a pandas.api.io
?
@@ -0,0 +1,61 @@ | |||
{%- block before_style -%}{%- endblock before_style -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure this is in the MANIFEST.IN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, only one significant comment on structure of template
pandas/formats/templates/html.tpl
Outdated
{%- endblock thead %} | ||
{%- block tbody %} | ||
<tbody> | ||
{%- for r in body %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a rows
block here? (or alternatively empty before_rows
and after_rows
). What I was trying to do is add additional rows at the end of the table which I don't think is possible with these extension points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas/__init__.py
Outdated
@@ -56,6 +56,7 @@ | |||
from pandas.util.print_versions import show_versions | |||
from pandas.io.api import * | |||
from pandas.util._tester import test | |||
from pandas.formats.style import Styler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it needs to be importable somewhere 'public', not sure if top level is the right spot, could argue either way. Do we need a pandas.api.io
?
pandas/formats/style.py
Outdated
@@ -400,7 +360,7 @@ def format(self, formatter, subset=None): | |||
self._display_funcs[(i, j)] = formatter | |||
return self | |||
|
|||
def render(self): | |||
def render(self, **kwargs): | |||
""" | |||
Render the built up styles to HTML | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add kwargs
behavior to docstring
From the notebook
|
Thanks.
About the API: I don't really want it at the top level but it has to be public if people are subclassing. I just wasn't sure how we resolved the public API and submodules issue :)
… On Apr 9, 2017, at 8:03 AM, chris-b1 ***@***.***> wrote:
From the notebook
For convenience, we provide the styler_factory function that does the same as the custom subclass.
styler_factory -> from_custom_template
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
yes I think this is the way to go. |
We already have a publicly accessible |
Yeah, having both would be confusing. Are people ok with |
ee5f953
to
5bff74f
Compare
Codecov Report
@@ Coverage Diff @@
## master #15954 +/- ##
==========================================
+ Coverage 91% 91% +<.01%
==========================================
Files 145 145
Lines 49581 49586 +5
==========================================
+ Hits 45123 45128 +5
Misses 4458 4458
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #15954 +/- ##
==========================================
- Coverage 91.02% 91% -0.03%
==========================================
Files 145 146 +1
Lines 50391 50416 +25
==========================================
+ Hits 45870 45880 +10
- Misses 4521 4536 +15
Continue to review full report at Codecov.
|
3c73d1c
to
bb58289
Compare
jinja2 being optional makes putting this in the namespace a bit strange. Happy to hear suggestions on ways to make that / the API tests better :/ |
from jinja2 import Template | ||
from jinja2 import ( | ||
PackageLoader, Environment, ChoiceLoader, FileSystemLoader | ||
) | ||
except ImportError: | ||
msg = "pandas.Styler requires jinja2. "\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do
try:
from jinja2 .....
except ImportError:
class Styler(ImportError):
pass
return
will give you a Styler
but if you try to use it and jinja2
is not installed will raise I think you can give it a nice message a well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback didn't quite work since you can't early-return from a module.
However, this does work:
try:
from pandas.formats.style import Styler
except:
from pandas.compat import add_metaclass
# We want to *not* raise an ImportError upon init
# We *do* want to raise an ImportError with a custom message
# when the class is instantiated or subclassed.
_msg = ("pandas.io.api.Styler requires jinja2. "
"Please install with `conda install Jinja2` "
"or `pip install Jinaj2`")
class _Check(type):
"""Raises import error upon subclassing."""
def __init__(cls, name, bases, clsdict):
if len(cls.mro()) > 2:
raise ImportError(_msg)
super(_Check, cls).__init__(name, bases, clsdict)
@add_metaclass(_Check)
class Styler(object):
def __init__(self, *args, **kargs):
raise ImportError(_msg)
Are we ok with a metaclass, just for a nice error message? :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh if that works great.
f18025d
to
ca0622c
Compare
pandas/api/io/__init__.py
Outdated
@@ -0,0 +1,2 @@ | |||
""" Public IO API """ | |||
from pandas.formats.style import Styler # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with having a pandas.api.io
, but isn't somewhat the opposite of the recommendation in #13634? Which is the top level modules would be 'public' and internals moved into core
- so this should be exposed as pd.io.Styler
? (I could be misunderstanding that issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas.io
is going to stay, so importing from pandas.io
is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, thought I removed that. Also might have just messed up my rebase. This isn't supposed to be there. It's supposed to be pandas.io.api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be just be pandas.io
? The second level api
are import *
into the top level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas.io.api
imports * to pandas
namespace; so you could make Styler
just show up there (or you can leave the from pandas.io.api import *
and just del Styler
(prob shorter)
Use Environment and PackageLoader to load it TODO: packaging, __init__.py? ENH: Add blocks to Styler template This will make subclassing the Styler and extending the templates easier. Jinja2 is optional, so Styler is maybe not there Nicer import failure Fixed rebase
As I said above, we already have public things in |
ca0622c
to
2ea04be
Compare
Ah, what @chris-b1 said :-) |
Currently, our API documentation lists it as If we decide to move the |
OK, should be good now. Sorry about that. |
I am shortly going to move a bunch of things around. But |
you could move |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api.rst will need an update as well, depending on what we decide
pandas/formats/style.py
Outdated
@@ -410,6 +370,11 @@ def render(self): | |||
------- | |||
rendered: str | |||
the rendered HTML | |||
**kwargs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to make the docstring a raw string (or escape the *'s), otherwise sphinx will complain :-)
doc/source/whatsnew/v0.20.0.txt
Outdated
@@ -482,6 +482,9 @@ Other Enhancements | |||
- ``DataFrame.to_excel()`` has a new ``freeze_panes`` parameter to turn on Freeze Panes when exporting to Excel (:issue:`15160`) | |||
- ``pd.read_html()`` will parse multiple header rows, creating a multiindex header. (:issue:`13434`). | |||
- HTML table output skips ``colspan`` or ``rowspan`` attribute if equal to 1. (:issue:`15403`) | |||
- ``pd.io.api.Styler`` template now has blocks for easier extension (:issue:`15649`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have an example in the notebook I suppose. Is it possible to put a link here to such an example? Not sure how it works with nbsphinx included files to put links to specific sections
@jorisvandenbossche yeah that worked! It's just
|
# We *do* want to raise an ImportError with a custom message | ||
# when the class is instantiated or subclassed. | ||
@_add_metaclass(_UnSubclassable) | ||
class Styler(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this jinja2
?
@@ -49,7 +49,8 @@ class TestPDApi(Base, tm.TestCase): | |||
'Period', 'PeriodIndex', 'RangeIndex', 'UInt64Index', | |||
'Series', 'SparseArray', 'SparseDataFrame', | |||
'SparseSeries', 'TimeGrouper', 'Timedelta', | |||
'TimedeltaIndex', 'Timestamp', 'Interval', 'IntervalIndex'] | |||
'TimedeltaIndex', 'Timestamp', 'Interval', 'IntervalIndex', | |||
'Styler'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can actually del Styler
from pandas
namespace
@TomAugspurger lgtm. merge when ready. |
FYI: http://pandas-docs.github.io/pandas-docs-travis/style.html#Fun-stuff is raising in built dev docs. |
|
Did you see that in one of the travis logs? https://travis-ci.org/pandas-dev/pandas/jobs/222148043 seems to be ok. |
@TomAugspurger see the link above, its in the built docs. |
Whoops, looked right past that. Yeah I guess they've split that out now. I can do now, or in a separate PR since all the checks already passed here. |
either way |
good to go? |
pandas/io/api.py
Outdated
@@ -17,6 +17,23 @@ | |||
from pandas.io.pickle import read_pickle, to_pickle | |||
from pandas.io.packers import read_msgpack, to_msgpack | |||
from pandas.io.gbq import read_gbq | |||
try: | |||
from pandas.formats.style import Styler | |||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImportError
thanks @TomAugspurger |
Opened a follow-up issue for the remaining issues that were being discussed: #16009 |
http://pandas-docs.github.io/pandas-docs-travis/style.html maybe internally should just catch warnings?
|