Codecov Report

Merging #541 into master will increase coverage by 0.04%.
The diff coverage is 98.41%.

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   97.45%   97.49%   +0.04%     
==========================================
  Files          29       29              
  Lines        2825     2875      +50     
==========================================
+ Hits         2753     2803      +50     
  Misses         72       72

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 218b6f8...4248327. Read the comment docs.

Worked out the memory profiling - you need to return the eval() at the end of _eval_once(), otherwise the output was always None. Thank you to @GuillaumeFavelier and @gzanitti for solving this!

Everything works - but the code is not elegant. Reviews welcome.

I should be able to look tomorrow. Glad you were able to figure out the eval/exec business in the meantime!

(I think this is a AppVeyor issue as all I changed were some variable names)

Looks good!

This should probably also be used at least once in the official docs. We use the gallery of sphinx-gallery itself to show functionality, so could you modify some example to show it, then once CircleCI renders it, paste a link here?

I would also remove the Closes tag at the top since this only takes care of the first of the two points in that issue

Thanks!

This should probably also be used at least once in the official docs. We use the gallery of sphinx-gallery itself to show functionality, so could you modify some example to show it, then once CircleCI renders it, paste a link here?

I can simply alter Local module so the code is:

N=100
N

It feels trivial but it shows the main point of the ENH. I would also like to add a note about the eval() in the getting started section. WDYT?

While looking into this I have realised that unwanted text is now output while making matplotlib plots -
eval("plt.ylabel('$\sin(x)$')") outputs Text(0, 0.5, '$\\sin(x)$') (as well as the graph), whereas exec("plt.ylabel('$\sin(x)$')") does not. This is also a problem in IPython/Jupyter (see SO). The workarounds are:

• to assign the output to a temp variable (_=plt.ylabel('$\sin(x)$'))
• add ; to the end of the expression
• use plt.show() as the last line

I have tested these and they all work here. I think the best/easiest solution is just to document this, either in the first gallery with a plot: Colourmaps or the Sin function. WDYT?

(On this note I would like to tidy up the galleries (e.g. Sine not Sin!(whoops)) and address #143 and I might do this before I tackle the __repr_html__)

I would also remove the Closes tag at the top since this only takes care of the first of the two points in that issue

I wanted to make this into 2 issues and 2 PRs but I can understand that it makes sense to discuss in 1 issue - in which case I could have just added to the end of the old issue..! Alas, I will remove 'closes'.

It feels trivial but it shows the main point of the ENH.

Sure, and once we support __repr_html__ you can add another or change it to an object (pandas dataframe?) with a more interesting one to show off that functionality.

I would also like to add a note about the eval() in the getting started section. WDYT?

Sure

While looking into this I have realised that unwanted text is now output while making matplotlib plots

This is going to be a potentially nasty backward compat break, as most libraries I work with do many such statements, and having the __repr__ printed every time will probably be annoing. We'll want a backward-compat config var for this, maybe:

print_eval_repr = ()

Does what master does (prints nothing ever based on the last line), while:

print_eval_repr = ('__repr_html__', '__repr__')

will print the __repr_html__ (to be added in another PR) if available, and if not available, will print the __repr__ of the line (to be done in this PR) of any line that does not contain an assignment/semicolon. Someday we can add finer-grained control by allowing things like # sg-(no-)print-eval-html-repr or something to enable disabling/enabling it on a line-by-line basis. But a global option to start is probably pretty good.

In MNE for example we will probably add __repr_html__ to our Report class, then set sphinx_gallery_conf['print_eval_repr'] = ('__repr_html__',), and have things work a reasonable way.

Sure, and once we support repr_html you can add another or change it to an object (pandas dataframe?) with a more interesting one to show off that functionality.

Yes that is a good idea. I think it would be useful to make a new .py file for this, as it doesn't seem to really fit with any of the other example galleries.

With regards to the Matplotlib problem - this bothered me all weekend but I could not think of a better solution. I have added the configuration.

A few points:

• In my mind, once we support __repr_html__, there would be 2 config options:
  • exec everything, as with master
  • exec everything except the last statement. If it is an expression eval it and capture __repr_html__ if it exists and if not just __repr__

Thus it does not make sense to me why we would make the config equal to () for the first option and ('__repr_html__', '__repr__') for the second. Unless you wanted 3 options, with the 3rd option being eval the last expression and capture __repr__ only, and never capture __repr_html__. Maybe I am missing something?

• With regard to finer grain control I think per code block control is adequate, since the config only changes what happens to the last statement in a code block anyway.

In MNE for example we will probably add repr_html to our Report class, then set sphinx_gallery_conf['print_eval_repr'] = ('repr_html',), and have things work a reasonable way.

I am not sure how this would work. I understand fine grain control to work like
# sphinx_gallery_line_numbers, which can be set on a file-by-file basis (except we would make it work by a per code block basis). Would the Report class add something like # sg-(no-)print-eval-html-repr to the source .py file?

For an existing Sphinx-Gallery project, I feel like it would be annoying to add this configuration (in new example .py files) to every code block, even if you just wanted to nicely print a pandas df nicely or not have to add print(), for example. File-by-file would be nice in this case, though I understand something like the Report class solution would allow easy migration to the new system for old .py files... Not sure of the best solution.

In my mind, once we support repr_html, there would be 2 config options:

1. exec everything, as with master
2. exec everything except the last statement. If it is an expression eval it and capture repr_html if it exists and if not just repr

I would not branch the code this way. I would always execute the second way, then the config option just determines which of the results we actually display in the output. Seems simpler to me.

Unless you wanted 3 options, with the 3rd option being eval the last expression and capture repr only, and never capture repr_html. Maybe I am missing something?

• We need () for backward compat.
• In MNE we would probably use ('__repr_html__',) so we only display objects with a nice HTML repr.
• Other packages will want ('__repr_html__', '__repr__') (display repr for every last line).
• And it's future compatible with displaying other things, e.g. people could do ('__repr_html__', '__str__', '__repr__') if they want.

Your code just needs to try to run the options methods they give in order and check if the result is str.

I am not sure how this would work.

Hopefully it makes sense based on what I said above

Would the Report class add something like # sg-(no-)print-eval-html-repr to the source .py file?

No we will add a __repr_html__ method to the Report class, set print_eval_repr = ('__repr_html__',) to our config, and add:

report = Report(...)
report  # <-- this line

To our examples.

I am not sure how this would work. I understand fine grain control to work like # sphinx_gallery_line_numbers, ... File-by-file would be nice in this case ...

Let's worry about this when we actually need it. For now let's work on sorting out how the main config option should work, then when someone hits a use case let's figure out the file/block/line-based fine-grained control.

I would not branch the code this way. I would always execute the second way, then the config option just determines which of the results we actually display in the output. Seems simpler to me.

To lay out more clearly, this is what I would do:

exec(all_but_last_line)
obj = eval(last_line)
last_str = None
for meth in sg_config['print_eval_repr']:
    try:
        last_str = getattr(obj, meth)()
    except Exception:
        pass
    else:
        if isinstance(last_str, str):
            break
if isinstance(last_str, str):
    append_as_rst(str)

I would not branch the code this way. I would always execute the second way, then the config option just determines which of the results we actually display in the output. Seems simpler to me.

Yes that makes much more sense. And trying the 'repr' methods in order of preference given by the config tuple sounds like an elegant solution. I will amend this.

With regard to the fine grain control, I was mostly asking how the config would be changed by the .py file but as you said we can worry about this later.

I was initially thinking to separate the 2 steps (as per #540) into 2 PRs (to keep it modular..?) but I can see also that it might make sense to do it all in 1 PR. (though this might depend on how I implement it) WDYT?

The problem is that I am having trouble working out how to capture anything else apart from __repr__(). At the moment I am evaluating (eval()) ast.Expression(body=code_ast.body.pop().value) (the value of last node of the parsed code_ast as a ast.Expression()). It returns the __repr__() by default. I have not worked out how to capture the other methods (e.g. _repr_html_, __str__) using ast.

In the #420 POC PR, it is done using IPython.display.display (which I think is here). I am not sure how this works and if I should be trying to implement in a similar way...

Suggestions welcome, otherwise I will continue banging my head against ast documentation...

code_ast.body.pop().value is the last line of code, right? If so then maybe the thing to do is see:

1. if there is an assignment (ast can probably tell you this?), then do nothing
2. if there is no assignment, make one to a temporary variable, then interrogate the methods of that temporary variable

No idea if this makes sense without digging into ast and eval, so feel free to ignore if it's nonsensical

Errant close, sorry

I can definitely check if it is an assignment. An expression is anything that can be on the RHS of a variable assignment (i.e., var1 = (anything that you can put here is an expression)). This means variable assignment, for loop, try/except are all NOT expressions (see SO). So if it is an expression (ast.Expr), it is definitely not an assignment.

The problem is that I am evaluating the compiled code_ast (code block parsed to ast) so I don't know how to do the assignment within the ast node. My 'hacky' ideas have involved altering the source code string but I think this is too dangerous. I will can ask around and explore more tomorrow...

My 'hacky' ideas have involved altering the source code string but I think this is too dangerous. I will can ask around and explore more tomorrow...

This is basically what I was thinking. Use '_' or '__' (or however many _ you need to get a var not in the namespace), assign to it, use it, then delete it. Agreed it's a hack but might be worth pursuing just to see that something can work.

I think (hope) this solution is an acceptable level of 'hacky'.

I worked out how to assign the last statement, if it is an expression to a variable called ___, using ast. Essentially what is happening is:

a = 2
a

turns into:

a = 2
___ = a

Technically, since only statements that can be on the RHS of an assign are expressions, I think I am safe in assigning any expression to a variable. Doing it with ast is also safer than trying to alter the code block string.

___ is now able to be accessed with example_globals['___']. (I realised that I could also use ast to get the name of the variable called in the last expression and then access it with example_globals, however not all last expressions will be a single variable (e.g., a+b or 1+2)).

I can then use ___ and the logic you described above to look for the 'reprs' as per the config tuple. A few points:

• the user cannot use the variable name ___ (I think this is reasonable)
• the memory profiler is giving the max memory of the assignment of the expression instead of just the expression (e.g., ___ = a instead of a). I don't know if how much difference there may be with this...?

html header

The html header I used was just the very simple:

.. only:: builder_html

    .. raw:: html

I think it is best for users to include in their raw html anything extra (e.g., if they want their html in an iframe). Open to suggestions.

Tests

I appreciate that I may have gone overboard with the tests. I tried to make it clean (thank you @massich for your help) but please suggest better solutions. Here are the cases I wanted to capture:

• Difference between () and ('__repr__',) with code including a print statement (in body and as last expression), with and without expression as last statement.
  • Side note, is it common knowledge that if you assign a print statement to a variable (e.g., a = print('hello')) and then try to get the __repr__ attribute (getattr(a, '__repr__')()) you get 'None' as a string? What is the reason for this?
• I could not think of any Python objects with a _repr_html_ (without having to add a dependency) so my code to test this includes defining a class with both __repr__ and _repr_html_ and just __repr__. I wanted to include cases that test that the config tuple order determines preference and that nothing is returned if the object did not have the methods included in the tuple.
• In cases where there is standard output (e.g. a print()) from the body and html output, there would need to be both a code block header and html header in the output.

CI is only failing for Python_version 'nightly' - and it is failing for many tests...I have no idea why...

(You were too quick for me! But here is my summary anyway, for clarity)
I have:

• changed the config name to capture_repr
• indented the html properly
• amended the html header to have line breaks at end, otherwise subsequent text was too close
• updated the configuration and getting started docs
• changed the config to 'capture_repr': ('_repr_html_', '__repr__')
• updated the examples that include matplotlib to avoid text output/explain why plt.show() was added
• added an example to for the 'capture_repr' config

Unfortunately, the pandas dataframe used in the example does not look nice because the raw html in _repr_html_ refers to external CSS. I am not sure where this CSS can be found and even if I could find it, I am not sure if this would be uniform for other packages. When inspecting a pandas dataframe rendered with jupyter notebook, it seemed that jupyter defines their own CSS (but I am not good with this so not certain). I think for MNE reports, the CSS is included with the html, so the report document is standalone, which would be great. Not sure of the best solution for this...

The test failing is because I added html line breaks. But I've also noticed an indenting problem. Might ponder overnight and do it tomorrow.

@GaelVaroquaux nice to hear from you! :p

The styling for pandas appears code-wise somewhere here:

https://github.com/pandas-dev/pandas/blob/master/pandas/io/formats/html.py
https://github.com/pandas-dev/pandas/blob/master/pandas/io/formats/style.py

But really it looks like their output already includes it. Looking at the HTML I see:

<style scoped>
    .dataframe tbody tr th:only-of-type {
        vertical-align: middle;
    }

    .dataframe tbody tr th {
        vertical-align: top;
    }

    .dataframe thead th {
        text-align: right;
    }
</style>
<table border="1" class="dataframe">
  <thead>
    <tr style="text-align: right;">
      <th></th>
      <th>col1</th>
      <th>col2</th>
    </tr>
  </thead>
  <tbody>
    <tr>
      <td>0</td>
      <td>1</td>
      <td>3</td>
    </tr>
    <tr>
      <td>1</td>
      <td>2</td>
      <td>4</td>
    </tr>
  </tbody>
</table>

How did you think it should look? If it looks better in Jupyter, it might be worth playing around a little bit with the code and/or pinging the pandas folks (listserv?) to get a hint on how to get _repr_html_ to include the necessary CSS styling required to look good. It might be a matter of calling some private function to make it do Jupyter-style outputs. We might end up needing to include some external CSS with the _repr_html_ calls, which we could do with another config var capture_repr_css that points to some file to use to inject more <style scoped> when rendering _repr_html_. It would be nice if we could get Pandas to do it for us, but if Jupyter is actually what makes it look nice, then we might need to inject their CSS.

(As we do more "it almost works but if only we could ..." fixes, we presumably get closer to replicating the functionality of nbconvert, though -- so at some point we might want to reconsider whether or not to use it.)

The dataframe looks slightly better in jupyter notebook:

I thought that class="dataframe" might refer to a CSS class defined elsewhere? Also <style scoped> seems to be a style, defined somewhere else?

If we do decide to just use nbconvert I might be slightly upset - (┛ಠ_ಠ)┛彡┻━┻ 😄 But a good proof of concept is right here: #422

I think CSS addition should be another PR...? But I am happy to ask about pandas.

🙃

Don't wait for another review from me. This is great!!

Thanks @lucyleeow !

A release coming anytime soon so we can use this?

support for _repr_html_ is in the docs and had me really confused why it wasn't working for me...

@banesullivan yes it's a good point, documentation is ahead of the current release - I've been meaning to create an issue about this.

If you do wish to use this feature you can install as developer. Sorry for the confusion.

I think it's fine to release now. @lucyleeow do you want to try following our instructions for how to make a release?

https://sphinx-gallery.github.io/maintainers.html#how-to-make-a-release

I should be able to get onto it this week but from reading the instructions, I may have some questions. What's the best way to ask? Make a new issue?

Should I do the items that don't require push access to SG or?

@lucyleeow send me your PyPi address and I'll add you there. Working on giving you write permissions here but I don't have admin access yet (I think?). Going to bother @agramfort about it

😮 that is a lot of trust.

I just made a PyPi account: https://pypi.org/user/lucyleeow/

Done. But let me get #559 taken care of before release

If you are taking care of #559 I would like to do #143 before the release (I've already started).