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

[DOC] Examples added to docstrings and minor fixes. #31

Merged
merged 9 commits into from
May 17, 2020

Conversation

ryanhammonds
Copy link
Contributor

Changes here are related to #20. The first three commits here are minor fixes. These can be changed or dropped easily. The other commits contain docstring examples for all API classes and functions.

@TomDonoghue
Copy link
Member

Some first pass thoughts:

  • I noticed some of the examples use the collected tutorial data. This is a nice idea, but the issue is that these only work when they are run from inside the module, with the github repo. Otherwise, from an installed version, or when copy/pasted, these examples don't work. I think a key goal of the examples should be that you can copy/paste the example, and it will run in a separate notebook or wtv, and then one can iterate on it from there. For that reason, I think we should not use the tutorial data in doctest examples.
  • Relatedly, some of the expected answers, including with tutorial data, expect and depend on information from a particular tutorial run, but this can change with new runs. So, for example, I cloned, installed the local dev version, and rebuilt the tutorials, and some of the doctest examples fail when comparing outputs. Expected outputs, if they are used, are going to need to be flexible to considering that scrapes can change when pubmed updates.
  • I think for some examples we can use 'fake' data (as in just create a data object filled in with something reasonable), and this also has the benefit of not launching too many scrapes.
    • This comes to mind, for example, for examples like compute_normalization in analysis/counts. This examples scrapes a 2-by-2 cooc array, but perhaps we could just create an array for this. Hopefully this is somewhat more direct, and focuses on the function of interest a bit more. To make the example data more 'real', maybe copy values from an actual run.
    >>> import numpy as np
    >>> coocs = np.array([[23, 52], [53, 12]])
    >>> counts = np.array([212, 46])
    
  • For doing examples with plots, I have somewhat mixed feelings, because these often end up being basically the same example as is in a different function (one that does the relevant computation) and then just adds the plot line at the end. This is basically copied code, and can be quite a lot just to get some data to plot. In these cases, I'm not sure we need the plot docstring example. One option, is perhaps we could actually call the plot in the example computation, and then in the plot call say 'see example in function, or something? (This is also relevant to NDSP).
  • So, this module is a difficult one to doctest. I don't think we need to try and overdo it to make a doctest example on every function / method. It's fine if some things end up being really tricky, and the example would just be too big, and in the end we don't add an example for some things (including if the update here removes some examples).
  • I think maybe let's not use trailing blank lines in the docstrings? Not really sure what the general convention would be, but for reason I think I prefer without. This doesn't matter for building the docsite.
  • I didn't know about tempfile, that's really useful / cool! Great idea!

@ryanhammonds : can you check through these comments, and edit / update from here. Perhaps most notably is to remove dependencies on the tutorials. Let me know if you want to chat through any of this, and when things are updated, tag me back in to re-review!

@codecov-io

This comment has been minimized.

@ryanhammonds
Copy link
Contributor Author

@TomDonoghue Alright, so I have dropped the tutorials data dependence by using doctest SKIP or "fake" data in the latest commit. Plotting examples have been moved as you suggested, with the exception plot_years, which seemed out of place in the init of ArticlesAll , which doesn't actually initialize with a years attribute. I also removed the blank trailing lines in the docstrings.

@TomDonoghue
Copy link
Member

Same as NDSP, I did a review, and it all looks good, but I decided it would be easier / friendly to do direct edits on some tweaks. @ryanhammonds - can you have a sanity check look at my commits, and make sure they seem sensible? If so, this should be good to go.

@ryanhammonds
Copy link
Contributor Author

@TomDonoghue I reviewed your pushed commits and everything looks good to me. Thanks!

@TomDonoghue TomDonoghue merged commit 36ee11c into lisc-tools:master May 17, 2020
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 this pull request may close these issues.

3 participants