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.

Sign up for GitHub

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

Jump to bottom

T000 - Talktorial template #85

Merged
merged 9 commits into from
Dec 15, 2020
Merged

T000 - Talktorial template #85

merged 9 commits into from
Dec 15, 2020

Conversation

dominiquesydow
Copy link
Collaborator

@dominiquesydow dominiquesydow commented Dec 15, 2020 •
edited
Loading

Details

TODO

Update template w.r.t. @AndreaVolkamer's suggestions (that she send via PM - copied here):

  • (it is self-explanatory, but one could add a note at the beginning that examples are taken from T001)
  • Practical: Short summay -> Short summary
  • In the path description make it even more explicit, that no relative paths should be given
  • Info for imports (copy info on ordering from issue (?))
  • maybe for later: generally we could add some general tips from the PR description in here, I will list a few here
    • Style guides black, Pep8, …
    • If functions are used, use docstrings, copy one example over? (one more comment on fcts (local/global variables - do not use globals in function) + link to docstring documentation
    • Or tips at the end:
      • Clear output and rerun, does it finish without errors?
      • Deterministic behavior/output? (help with CI comments)
      • Check runtime?
  • In talktorial practical header repeat once that one should make sure the titles are the same as above (as well as the indentation)

@dominiquesydow dominiquesydow self-assigned this Dec 15, 2020
@dominiquesydow dominiquesydow mentioned this pull request Dec 15, 2020
Merged
27 tasks
@dominiquesydow
Copy link
Collaborator Author

Hi @AndreaVolkamer ,

This PR is ready for your review.
https://github.com/volkamerlab/teachopencadd/blob/ds-t000-revamp/teachopencadd/talktorials/T000_template/talktorial.ipynb

I did not add an general comment on

Style guides black, Pep8, …

since I think that Jaime or Jeff wanted to set up a document on best practices anyways, which we will be able to link here in the future.

I did referring though to (a) specific PEP8 guidelines e.g. for imports and (b) our template issue.

AndreaVolkamer
AndreaVolkamer reviewed Dec 15, 2020
View reviewed changes
Copy link
Member

@AndreaVolkamer AndreaVolkamer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@dominiquesydow awesome, I really like the added information and the highlighted boxes!
I added only very few minor suggestions, feel free to include them. The notebook can afterwards be merged without further approval from my side.

teachopencadd/talktorials/T000_template/talktorial.ipynb Outdated
"cell_type": "markdown",
"metadata": {},
"source": [
"_The examples used in this talktorial template are taken from __Talktorial T001__._"
Copy link
Member

Choose a reason for hiding this comment

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

maybe include an actual link to T001?

teachopencadd/talktorials/T000_template/talktorial.ipynb Outdated
"source": [
"<div class=\"alert alert-block alert-info\">\n",
" \n",
"<b>Figures</b>: Place images in the <code>images/</code> folder and include them using Markdown <code>![Figure title](images/some_figure.jpg)</code>. Add a figure caption in the format provided below.\n",
Copy link
Member

Choose a reason for hiding this comment

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

.. in the format provided above (not below)?

teachopencadd/talktorials/T000_template/talktorial.ipynb Outdated
Comment on lines 335 to 340
"<b>Relative paths</b>: Please do not use relative paths to example input/output data or images.\n",
"Instead add this talktorial's path to the global <code>HERE</code>. Define all paths relative to this path.\n",
"\n",
"<br/>\n",
" \n",
"If your talktorial has input/output data, please define the global <code>DATA</code>, which points to this talktorial's data folder (check out the default folder structure of each talktorial).\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"<b>Relative paths</b>: Please do not use relative paths to example input/output data or images.\n",
"Instead add this talktorial's path to the global <code>HERE</code>. Define all paths relative to this path.\n",
"\n",
"<br/>\n",
" \n",
"If your talktorial has input/output data, please define the global <code>DATA</code>, which points to this talktorial's data folder (check out the default folder structure of each talktorial).\n",
"<b>Relative paths</b>: Please define all paths relative to this talktorial's path by using the global variable <code>HERE</code> and define all paths relative to this path.\n",
"\n",
"<br/>\n",
" \n",
"E.g. if your talktorial has input/output data, please define the global <code>DATA</code>, which points to this talktorial's data folder (check out the default folder structure of each talktorial).\n",

Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion, I felt the before sentences were somehow duplicated.

teachopencadd/talktorials/T000_template/talktorial.ipynb
" [molecular_weight, n_hba, n_hbd, logp, ro5_fulfilled],\n",
" index=[\"molecular_weight\", \"n_hba\", \"n_hbd\", \"logp\", \"ro5_fulfilled\"],\n",
" )"
]
Copy link
Member

Choose a reason for hiding this comment

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

Could we add before/after the example function, to use meaningful function and parameter names?

teachopencadd/talktorials/T000_template/talktorial.ipynb Outdated
"<b>Useful checks at the end</b>: \n",
" \n",
"<ul>\n",
"<li>Clear output and rerun your runtime. Does it finish without errors?</li>\n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"<li>Clear output and rerun your runtime. Does it finish without errors?</li>\n",
"<li>Clear output and rerun your complete notebook. Does it finish without errors?</li>\n",

@dominiquesydow
Copy link
Collaborator Author

Great, thanks, @AndreaVolkamer - all your suggestions are integrated. Will merge now.

@dominiquesydow dominiquesydow merged commit 0a3b823 into t011-base Dec 15, 2020
@dominiquesydow dominiquesydow deleted the ds-t000-revamp branch December 15, 2020 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreaVolkamer AndreaVolkamer AndreaVolkamer left review comments

Assignees

@dominiquesydow dominiquesydow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dominiquesydow @AndreaVolkamer