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

Simplify code review of tutorials #3930

Closed
jngrad opened this issue Oct 3, 2020 · 6 comments · Fixed by #3993
Closed

Simplify code review of tutorials #3930

jngrad opened this issue Oct 3, 2020 · 6 comments · Fixed by #3993
Assignees

Comments

@jngrad
Copy link
Member

jngrad commented Oct 3, 2020

Currently, solution cells contain python code wrapped in fenced blocks in markdown cells. The idea was to prevent the user from running the solutions by clicking on Run All. However, during hands-on sessions, tutors make sure the participants go through the cells one by one. During self-teaching, participants would presumably also go through cells one by one, because clicking Run All would put them at risk of triggering one of the the many assertion statements written between solutions to check participants' code.

Having the solution code wrapped in markdown cells makes it more difficult for tutorial authors and reviewers to run the solutions and edit them. It also makes the CI logic for tutorials more complex and fragile - we missed a bug in #3872, which had to be fixed in a tutorial PR (d6b4d97). It's also unclear - without inspecting the python code - what should happen when a solution markdown cell contains a fenced python code block with regular text around it. For future reference, here is the relevant python logic:

# convert solution markdown cells into code cells
if cell['cell_type'] == 'markdown' and 'solution2' in cell['metadata'] \
and 'solution2_first' not in cell['metadata']:
lines = cell['source'].strip().split('\n')
if lines[0].startswith(
'```python') and lines[-1].startswith('```'):
source = '\n'.join(lines[1:-1]).strip()
nb['cells'][i] = nbformat.v4.new_code_cell(source=source)

It would be simpler from a maintenance perspective to leave code solutions as code cells. This requires no change to CI.

If we approve this simplification, the snippet of code above becomes unused and can be removed in the distant future.

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Oct 3, 2020 via email

@jngrad
Copy link
Member Author

jngrad commented Oct 3, 2020

I find it important that solutions are hidden and that students who do the tutorials seriously attempt the exercises without seeing the solution.

This is probably a misunderstanding. This issue is not about hiding solutions, which we agree is important for didactic reasons. This is about making our life easier as maintainers and reviewers. Right now, to review and improve a tutorial, I have to convert all markdown cells to code cells, remove the backticks, do the changes and run the cells, add backticks, convert to markdown cells. Repeat this process every time you change something.

So I’m not too worried about serious issues not getting noticed in time.

The bug I mentioned was caught by chance. It only occurs once in a week, when we deploy tutorials to the ESPResSo website.

@RudolfWeeber
Copy link
Contributor

RudolfWeeber commented Oct 4, 2020 via email

@jngrad
Copy link
Member Author

jngrad commented Oct 8, 2020

It's no longer an issue from my side, thanks to tooling:

# convert solution cells to hidden code cells
/tikhome/jgrad/Documents/protocols/DevOps/exercise2/converter.py --to_py -i 04-lattice_boltzmann_part2.ipynb
# convert solution cells to hidden markdown cells
/tikhome/jgrad/Documents/protocols/DevOps/exercise2/converter.py --to_md -i 04-lattice_boltzmann_part2.ipynb

@RudolfWeeber
Copy link
Contributor

Offlien discussion: JN's scripts to do the cell type conversion will be integrated

@KaiSzuttor
Copy link
Member

what's the conclusion here?

@jngrad jngrad self-assigned this Nov 2, 2020
@kodiakhq kodiakhq bot closed this as completed in #3993 Nov 16, 2020
kodiakhq bot added a commit that referenced this issue Nov 16, 2020
Description of changes:
- remove crystallization tutorial (partial fix for #3955, closes #3820)
- remove tutorial numbers and indicate difficulty level (fixes #3959)
- resolve issues in tutorial LJ (fixes #3934)
- create a tool to simplify editing exercise2 notebooks by temporarily converting solution markdown cells to executable code cells (fixes #3930) and apply pep8 formatting
  ```sh
  # convert solution cells to code cells
  ./pypresso doc/tutorials/convert.py exercise2 --to-py doc/tutorials/lennard_jones/lennard_jones.ipynb
  # edit the notebook
  ./ipypresso notebook
  # convert solution cells back to markdown cells
  ./pypresso doc/tutorials/convert.py exercise2 --to-md doc/tutorials/lennard_jones/lennard_jones.ipynb
  # apply autopep formatting rules
  ./pypresso doc/tutorials/convert.py exercise2 --pep8  doc/tutorials/lennard_jones/lennard_jones.ipynb
  ```
- implement an autocorrelation function `espressomd.analyze.autocorrelation()` to calculate the ACF of 1-dimensional and 2-dimensional data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants