QuantEcon / sphinxcontrib-jupyter

A Sphinx Extension for Generating Jupyter Notebooks
BSD 3-Clause "New" or "Revised" License
76 stars 23 forks source link

ENH: enhance the exercise directives and add new exerciselist directive #239

Closed sglyon closed 5 years ago

sglyon commented 5 years ago

Hi team

This PR upgrades the exercise directive and adds an additional exerciselist directive.

The idea is that you can sprinkle exercsies throughout the lectures, wherever they make sense in context.

If you set a config value (in conf.py) exercise_include_exercises (default is True) they will be included in the output. If you change that to False, then the exercises will be removed when you generate the output

If you also include

.. exerciselist::

somewhere in the file, all exercises from that file will be produced at the point where you entered the .. exerciselist directive.

If the config variable exercise_inline_exercises is set to false (the default), then whereever the exercise initially appeared in the body of the document, you will instead see a link that says "See exercise # in the exercise list" where exercise list is a link to the list of exercises.

Beneath each exercise in the list there is another link "(back to text)" that points back to the location where you originally authored/placed the exercise.

There are a few options for exercise list...

.. exerciselist::
    :scope: SCOPE
    :from: FILE
    :force:

The following configurations are possible:

There is a little more logic:

This is needed for the imminent release of the datascience lectures, so a speedy review would be appreciated. If not we can work from this branch when we build and deploy.


That was probably a bit unclear. Check out the tests for usage examples.


cc @cc7768 and @mmcky

sglyon commented 5 years ago

test failures here are have two factors:

  1. I assume python 3.6+ (use f strings)
  2. I use some sphinx features only available in the 2.0+ major release series.

I can remove the use of f"{variable}" if we need to support old Pythons, but I don't think it will be easy for me to use an old sphinx... Does this cause problems for anyone?

jstac commented 5 years ago

Nice, thanks @sglyon !

It will be great to have this functionality. I'll leave to @mmcky and @AakashGfude to review.

mmcky commented 5 years ago

@sglyon thanks for an great addition to the extension and a really well documented PR. I will use the PR description to update the docs before we merge this in.

One issue with the sphinx>=2.0+ request is I know sage is using this extension and they are using python=2.7 which sphinx>=2.0 doesn't support so we could add a feature flag on the sphinx version and add to docs that the exercises feature is only available in sphinx>2.0

@sglyon I propose we use sphinx_version to import exercise nodes for major version >= 2 and adjust tests not to run exercise for sphinx<=2 and python<3.6

mmcky commented 5 years ago

@sglyon I have made a few changes to include version support for sphinx>=2.0 and the exercise feature. If you can review that would be great.

One bug is that the check_diffs.py script in tests doesn't parse the subfolders in each test set (i.e. base) so technically section2 is not getting tested at the moment. I am happy to open an issue to add support for subdir checking so we don't hold up merging this PR.

Also in the testset I have configured it to exclude checking the index files for any sphinx<2 as that way we check the index file generated on sphinx >= 2 that includes the exercise toctree and we only need to track one reference solution. I don't think it is necessary to have version specific solutions in base/ipynb at this stage.

mmcky commented 5 years ago

@sglyon does the exercise directive support nested directives? (i.e. can you add code-blocks to your exercise?)

sglyon commented 5 years ago

does the exercise directive support nested directives? (i.e. can you add code-blocks to your exercise?)

Yep, you can put arbitrary content into an exercise block. I use it heavily in the datascience lectures (which I used as a test to dogfood development here). That's a great point to add into the test cases. I've never tried nesting an exercise in an exercise but in principle it would resolve recursively.

When I come across an exercise node I actually just grab all the contents of the block and tell sphinx to reparse/process the contents here

I propose we use sphinx_version to import exercise nodes for major version >= 2 and adjust tests not to run exercise for sphinx<=2 and python<3.6

Sounds great to me

Also in the testset I have configured it to exclude checking the index files for any sphinx<2 as that way we check the index file generated on sphinx >= 2 that includes the exercise toctree and we only need to track one reference solution. I don't think it is necessary to have version specific solutions in base/ipynb at this stage.

Also works for me


There are two other features that I would like to make that relate to this:

  1. The ability to add a tag or label to an exercise like this:

    .. exercise::
    :label: pandas_groupby_1
    
    Here is the content...

    Then we would be able to refer to these blocks in an exercise list:

    .. exerciselist::
    :labels: pandas_groupby_1, pandas_groupby_2

    which would cause only those two exercises to be included in the list. This way instructors can piece together assignments by pulling at will from our bank of exercises without having to repeat the text

  2. Add a solutions directive. This would leverage the label feature of the exercise so that you can only have one unique solution for each exercise and you link them by label (after parsing warnings will be issues for any duplicate labeled solutions or any solutions missing an exercise). You would control the inclusion of the exercises using a config option in conf.py exercise_include_solutions where the default value is False

I think it would only take me an hour or two to put this together.I think may do this really quick and add to the PR while I have your ear on this feature.

What do you think?

sglyon commented 5 years ago

@mmcky actually you can see content in an exercise in the first test in exercises.rst here

mmcky commented 5 years ago

I think it would only take me an hour or two to put this together.I think may do this really quick and > add to the PR while I have your ear on this feature.

What do you think?

Please go ahead if you have time -- happy to work with you on this PR. I have tagged this as 0.4.1 which is due to release tomorrow but can delay a day or so to make sure it get's included.

mmcky commented 5 years ago

We do have some light weight support in the translator_code for :class: solution which allowed you to drop code-block solutions based on a :class: solution tag. Perhaps we should deprecate this in a future release to reduce confusion?

One other question for exercise directive:

  1. Do you think we need to add :title: option? Currently the default is to write Exercise {#} right? Just wondering if someone wanted to title it Question ({#}) etc.
mmcky commented 5 years ago

also I see you have html and pdf node support also. This is great! I will add that into the documentation section on directives.

mmcky commented 5 years ago

Is there an example for exercise_inline_exercises? Just adding all options to the docs

sglyon commented 5 years ago

Is there an example for exercise_inline_exercises? Just adding all options to the docs

Because this is an app-level config variable I wasn't sure how to toggle it on or off in the tests.

I think what we will need to do is probably create another really small sphinx project in the test directory that toggles that option off so we can verify that it works properly. I will work on that

sglyon commented 5 years ago

Do you think we need to add :title: option? Currently the default is to write Exercise {#} right? Just wondering if someone wanted to title it Question ({#}) etc.

Good idea. I'll try to implement this


i think for now I am going to punt on the solution concept. It shouldn't be too difficult to program up, but I'm already well over my budgeted time for this mini-project right now so I think I'll be disciplined and quit while I'm ahead

mmcky commented 5 years ago

Because this is an app-level config variable I wasn't sure how to toggle it on or off in the tests.

I think what we will need to do is probably create another really small sphinx project in the test > directory that toggles that option off so we can verify that it works properly. I will work on that

I don't have a great solution for this in place but what I have so far on this is the testset approach including base, pdf etc. which have independent conf.py and solution ipynb sets.

You could add exercise-default, exercise-a etc. and add a README file to document what each set is for.

Testing app level options is something I'd like to improve if you have any ideas.

sglyon commented 5 years ago

Haha nope, no ideas. Just brute force cp a bunch of files and change the config option

mmcky commented 5 years ago

i think for now I am going to punt on the solution concept. It shouldn't be too difficult to program up, but I'm already well over my budgeted time for this mini-project right now so I think I'll be disciplined and quit while I'm ahead

No problem. We can open an issue to track it as an feature request.

sglyon commented 5 years ago

phew ok I think I am done now.

Ready for review!

mmcky commented 5 years ago

It would be nice not to duplicate all the source files from base to no_inline_exercise test set. I will put an issue together to improve on this. What would be better is to use -D overrides on a common set of files with additional reference ipynb files.

configuration = {
  base : [("exercise_inline_exercises = False", ipynb/<reference ipynbs)]
}

We could then have a matrix that runs:

sphinx-build  BASE BASE/_build -D exercise_inline_exercises = False

and run that over base test set with different configuration options for comparison with different reference ipynb files.

Issue https://github.com/QuantEcon/sphinxcontrib-jupyter/issues/241 setup to track this.

mmcky commented 5 years ago

Added https://github.com/QuantEcon/sphinxcontrib-jupyter/issues/242 to track solution feature request.