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

Docs: Add first draft for tutorial #4070

Merged
merged 10 commits into from
May 27, 2020
Merged

Conversation

mbercx
Copy link
Member

@mbercx mbercx commented May 13, 2020

First draft of the AiiDA tutorial (See Issue #3981). Next to adding the tutorial itself, I've also:

  • Changed the ArithmeticAddCalculation to set defaults for the resources and use a basic bash execution on the input files.
  • Added the MultiplyAddWorkChain for demonstration purposes.
  • Set the background-color of bash code snippets to aliceblue.
  • Added the sphinx-copybutton extension.

Some outstanding issues/questions:

  • The sphinx-copybutton extension works fine, but perhaps it would be better if we can specify which code snippets the copy button is added to. Now using the copy button for many verdi shell snippets also copies the prompt. You can specify one prompt that is not included when copying the snippet contents, which I’ve currently set to ‘$ ‘ for the terminal snippets.
  • The work directory of the tutor computer is now set up under $PWD/work. I figured it might be nice to have them explore the contents of this directory at the end of the tutorial, so they can see how AiiDA uses the UUID to organise the directories.
  • Currently, the tutorial does not include querying.

I have gone through the tutorial myself in 13 minutes (reading the text aloud to slow me down a bit). I think the size is ok at this point, but we might want to cut some parts (if possible) and still include an example on querying.

@mbercx mbercx requested a review from csadorf May 13, 2020 12:58
@csadorf csadorf changed the title docs: add first draft of aiida tutorial Docs: add first draft of aiida tutorial May 13, 2020
@csadorf csadorf changed the title Docs: add first draft of aiida tutorial Docs: Add first draft for tutorial May 13, 2020
@sphuber
Copy link
Contributor

sphuber commented May 13, 2020

Thanks @mbercx . Since this change also changes existing code and adds the workflow, I wonder if we should add this in a separate commit straight onto develop. Since there are no tests in the docs-revamp that will depend on these changes, there are no problems to then have this only develop until we merge docs-revamp.

@mbercx
Copy link
Member Author

mbercx commented May 14, 2020

I have moved the changes to the ArithmeticAddCalculation, as well as the new MultiplyAddWorkChain, to a different branch. Once I've adjusted the tests and am able to run them successfully locally, I'll rebase this branch onto the develop branch and do the PR there.

I have also removed the sphinx-copybutton extension for now, as @chrisjsewell is working on this (see #4062). I'll also leave the issue I had regarding the prompt there.

@csadorf csadorf added the pr/on-hold PR should not be merged label May 14, 2020
@csadorf
Copy link
Contributor

csadorf commented May 19, 2020

@mbercx Sorry, this fell off my radar. Is this ready for my review?

@mbercx
Copy link
Member Author

mbercx commented May 19, 2020

@mbercx Sorry, this fell off my radar. Is this ready for my review?

No worries! The text is ready for review, yes, but note that not all of the code snippets will work as e.g. the MultiplyAddWorkChain has been removed from this PR to commit it straight into develop.

I have also used the code-block directive for Python code snippets here, but later noticed that many other parts of the documentation use a double colon ::at the end of the preceding paragraph. Have we established a convention for this?

@csadorf
Copy link
Contributor

csadorf commented May 19, 2020

@mbercx Sorry, this fell off my radar. Is this ready for my review?

No worries! The text is ready for review, yes, but note that not all of the code snippets will work as e.g. the MultiplyAddWorkChain has been removed from this PR to commit it straight into develop.

Ok, that's not a problem, because I did not plan on testing the tutorial before we are further down the line anyways.

I have also used the code-block directive for Python code snippets here, but later noticed that many other parts of the documentation use a double colon ::at the end of the preceding paragraph. Have we established a convention for this?

Good point, we had not established a convention for that yet. I suggest that we just use a double colon. Unless there are objections I will put that into the wiki page and ensure consistency prior to merging of the docs-revamp branch.

@sphuber
Copy link
Contributor

sphuber commented May 19, 2020

Good point, we had not established a convention for that yet. I suggest that we just use a double colon. Unless there are objections I will put that into the wiki page and ensure consistency prior to merging of the docs-revamp branch.

The only question I have there is weather the double colon will robustly determine the correct language for the highlighting. Even if that is guaranteed, having an explicit .. code-block:: with explicit language might also be a lot clearer for the reader of the raw docs.

@csadorf
Copy link
Contributor

csadorf commented May 19, 2020

Good point, we had not established a convention for that yet. I suggest that we just use a double colon. Unless there are objections I will put that into the wiki page and ensure consistency prior to merging of the docs-revamp branch.

The only question I have there is weather the double colon will robustly determine the correct language for the highlighting. Even if that is guaranteed, having an explicit .. code-block:: with explicit language might also be a lot clearer for the reader of the raw docs.

It is assumed to be Python. If I understand this correctly then one would use .. highlight:: directive to control the syntax highlighting and other things like line numbers.

On the other hand, .. codeblock:: requires one to explicitly set the language every time, right? I'm honestly not sure what is better here.

@sphuber
Copy link
Contributor

sphuber commented May 19, 2020

Since we sometimes also have bash blocks, I would opt for consistency and clarity and use .. code-block:: python.

@mbercx
Copy link
Member Author

mbercx commented May 19, 2020

Even if that is guaranteed, having an explicit .. code-block:: with explicit language might also be a lot clearer for the reader of the raw docs.

As someone who had little experience with reStructuredText before we started working on the docs revamp, I second this notion!

Also, I just remembered that if we are using the sphinx-copybutton extension, as per @chrisjsewell's suggestion, you can use a CSS selector to indicate that a code block should not have a copybutton, see issue #4062.

@csadorf
Copy link
Contributor

csadorf commented May 19, 2020

Ok, then it's settled .. code-block:: it is, I'll update the wiki.

@chrisjsewell
Copy link
Member

chrisjsewell commented May 19, 2020

While we are at it, perhaps in the wiki, we want to add some extra clarification over writing code blocks,
i.e. IMO there is probably 4 distinct types of code block that we use a lot:

  • Bash shell scripts should use bash
    .. code-block:: bash
    
        verdi node show 1
  • Bash shell sessions (i.e. interactive) should use console
    .. code-block:: console
    
        $ verdi node show 1
  • Python scripts should use python
    .. code-block:: python
    
        from aiida import orm
  • Python sessions (i.e. via verdi shell) should use ipython
    .. code-block:: ipython
    
        In  [1]: print("a")
        Out [1]: "a"

Also in the guidelines

Use the * symbol for itemized lists

This seems like a weird symbol to use for itemized, since it is also used for emphasis/bold. Would - not be better?

@csadorf
Copy link
Contributor

csadorf commented May 19, 2020

While we are at it, perhaps in the wiki, we want to add some extra clarification over writing code blocks,

Yes, this actually came up in another PR as well. I would just go ahead and copy your examples if you don't mind.

Also in the guidelines

Use the * symbol for itemized lists

This seems like a weird symbol to use for itemized, since it is also used for emphasis/bold. Would - not be better?

I just made a decision, I believe based on prevalence. I don't really think that using * is a problem.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great! I think that having a tutorial like this will make the on-boarding of new users much, much easier. Thank you!

I've added plenty of suggestions to improve flow and wording and also to fix a few typos here and there. Additionally, I'd like to ask you to streamline some of the prompts:

  • Make sure that the code-blocks are marked up as shown here.
  • Make sure that the user always knows whether code is supposed to be executed in a script or within a Python interpreter. I found some ambiguity with respect to that.

docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
@csadorf csadorf linked an issue May 20, 2020 that may be closed by this pull request
@mbercx
Copy link
Member Author

mbercx commented May 22, 2020

Thanks a lot for the review, @csadorf! Although I'm not sure what connotation of handy you are referring to. ^^

  • Make sure that the code-blocks are marked up as shown here.
  • Make sure that the user always knows whether code is supposed to be executed in a script or within a Python interpreter. I found some ambiguity with respect to that.

As all code snippets are supposed to be run in the verdi shell, I adjusted them to make this clear to the user. However, copying code snippets with multiple lines is now very tedious manually, so we should probably add a note to point out the sphinx-copybutton's to make sure users don't miss them, once they have been integrated with regexes for the prompts.

@csadorf
Copy link
Contributor

csadorf commented May 25, 2020

@mbercx Is this ready for another round of review?

@mbercx mbercx requested a review from csadorf May 25, 2020 11:07
@mbercx
Copy link
Member Author

mbercx commented May 25, 2020

@mbercx Is this ready for another round of review?

Yes! Sorry, I hadn't discovered the re-request review button yet. ^^

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! This is almost ready to merge.

docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
@csadorf
Copy link
Contributor

csadorf commented May 25, 2020

Please also resolve the merge conflicts.

docs/source/conf.py Outdated Show resolved Hide resolved
mbercx added 3 commits May 26, 2020 12:56
First draft of the AiiDA tutorial. Next to adding the tutorial itself, I've also:
* Changed the ArithmeticAddCalculation to set defaults for the resources and use a basic bash execution on the input files.
* Added the MultiplyAddWorkChain for demonstration purposes.
* Set the background-color of bash code snippets to aliceblue.
* Added the sphinx-copybutton extension. This still needs to be improved, as currently all code snippets are provided with a copy button, and e.g. those with a Python prompt also copy this prompt.
Remove the MultiplyAddWorkChain class and the changes to the
ArithmeticAddCalculation class from this branch, so they can be merged
separately into the develop branch.

Also remove the sphinx-copybutton extension for now, as this addition
is being dealt with in another issue (aiidateam#4062).
Replace the literalinclude for the MultiplyWorkChain with a code-block
as the module has been removed from this branch.

Also, clean up the trailing whitespace.
chrisjsewell and others added 4 commits May 26, 2020 12:57
Co-authored-by: Carl Simon Adorf <[email protected]>
Use proper code-block formatting for the different code-block types.

Add aliceblue background color for console code-blocks.

Add output for `verdi daemon status` command, as well as small fixes
to the text.
@mbercx mbercx requested a review from csadorf May 26, 2020 12:05
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! I found a few typos etc. and have one question, but overall I think we're close to done.

docs/source/tutorial/basic.rst Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved
docs/source/tutorial/basic.rst Outdated Show resolved Hide resolved

.. code-block:: ipython

In [1]: from aiida.workflows.multiplyadd import MultiplyAddWorkChain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use a factory function here instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I didn't know how to add an entry point when I first wrote the tutorial. ^^ I've also added a short sentence on the WorkflowFactory. Have a look and see if you think it's clear/sufficient.

@mbercx mbercx requested a review from csadorf May 26, 2020 19:26
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@csadorf csadorf merged commit 2101a3b into aiidateam:docs-revamp May 27, 2020
@mbercx mbercx deleted the docs-revamp branch May 29, 2020 12:22
csadorf added a commit that referenced this pull request May 29, 2020
Add a tutorial to teach new users basic AiiDA concepts.

Co-authored-by: Chris Sewell <[email protected]>
Co-authored-by: Carl Simon Adorf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/on-hold PR should not be merged topic/documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Create a new tutorial for first-time users
4 participants