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

Better doc #82

Merged
merged 54 commits into from
Feb 20, 2018
Merged

Better doc #82

merged 54 commits into from
Feb 20, 2018

Conversation

amnona
Copy link
Collaborator

@amnona amnona commented Feb 14, 2018

improve sphinx documentation:

  • add nbsphinx for integrating jupyter notebooks into sphinx doc
  • create jupyter notebooks for calour tutorials
  • add **kwargs documentation using docrep (work in progress - some are not working)

Remove travis sphinx error which is due to sphinx-autodoc-typehints and the way we add functions to the Experiment class. Currently just removed sphinx-autodoc-typehints. Maybe can fix deeper

Change the Experiment.repr to remove the sample and feature list since in most cases it is huge, ugly and not useful

Add random_seed to diff_abundance to enable totally reproducible results.

and small goodies


Returns
-------
newexp : :class:`.Experiment`
The experiment with only significant (FDR<=maxfval) difference, sorted according to difference
'''
if random_seed is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the behavior if it is None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it says in the docstring... added also a comment before this line.
The idea is if None, don't touch it (so each time you get random results)

try:
lines.append('feature IDs: %r' % self.feature_metadata.index)
except AttributeError:
lines.append('No feature metadata')
Copy link
Collaborator

Choose a reason for hiding this comment

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

you think it prints too much? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

then could you rewrite the printout? like we don't need ------- any more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to:
class (description) with X samples, Y features

@@ -422,6 +422,10 @@ def filter_abundance(exp: Experiment, min_abundance, **kwargs):
min_abundance : numeric
The minimal total abundance for each feature over all samples

Other parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's call this kwargs parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"Other parameters" is hard coded into docrep (part of the numpy doc standard)
Since docrep is just one file (the init.py), we can copy it into calour and then change it so it will also include kwargs parameters etc.
I think it will also be better to include it in calour since then we remove the dependency on a (very small) package that may change in the future without us knowing.
Alternatively, we can fork docrep and use the fork for installation (but then install will be via github... but that's also fine...).
or we can use the current docrep from its repository and keep it as Other Parameters...

what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok - let's copy it? if license allows

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

license is currently GPL-2. I posted a question to the author if it's ok to copy/modify and use in our BSD-3 project. Let's hope he says it ok :).


Parameters
----------
cutoff : float (optional)
The minimal mean abundance fraction (out of the mean of total abundance per sample) for a feature in order
to keep it. Default is 0.01 - keep features with mean abundance >=1% of mean total abundance per sample

Other parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

same



logger = getLogger(__name__)


def _create_plot_gui(exp, gui='cli', databases=('dbbact',), tree_size=0):
def _create_plot_gui(exp: Experiment, gui='cli', databases=('dbbact',), tree_size=0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not need type hint of ": Experiment", but it does not hurt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed for consistency

This GUI only enables the zooming/panning and displays information following a mouse click
in the terminal window

Other Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

mark

def __init__(self, *kargs, **kwargs):
'''Init the GUI using the Qt5 framework.

Other Parameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

mark

@@ -63,6 +67,10 @@ def sort_centroid(exp: Experiment, transform=log_n, inplace=False, **kwargs):
kwargs : dict
additional keyword parameters passed to ``transform``.

Other Parameters
----------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

mark

@@ -14,3 +14,4 @@ ipywidgets
numpydoc
sphinx
sphinx_bootstrap_theme
pandoc
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this is required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pandoc is required for the nbsphinx. since nbsphinx is not in conda, we pip install it, but better to have pandoc from conda?
or do you think there's a better way to do it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is good.


# See Also
# --------
# :class:`sklearn.preprocessing.OneHotEncoder`
Copy link
Collaborator

Choose a reason for hiding this comment

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

understand you changed repr, but why other lines of docstring are commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uncommented and fixed to new repr results

@RNAer
Copy link
Collaborator

RNAer commented Feb 15, 2018

and simple.ipynb is not used?

@RNAer
Copy link
Collaborator

RNAer commented Feb 15, 2018

should we remove or merge the notebooks under root dir with what are inside doc?

@amnona
Copy link
Collaborator Author

amnona commented Feb 15, 2018

ok, fixed everything except the "Other Parameters" (since hardcoded in docrep) and the location for the docs notebooks

calour/io.py Outdated
@@ -466,6 +382,90 @@ def read_amplicon(data_file, sample_metadata_file=None,
return exp


@ds.with_indent(4)
def read_open_ms(data_file, sample_metadata_file=None, gnps_file=None, feature_metadata_file=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this whole function is relocated? it's hard to see if there is any changes...

Copy link
Collaborator Author

@amnona amnona Feb 16, 2018

Choose a reason for hiding this comment

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

just relocated since i think otherwise docrep has problems since the function for which we need the kwargs parameters list is after the function that needs to print them.
not changes otherwise

@amnona
Copy link
Collaborator Author

amnona commented Feb 16, 2018

ok.
ready for merge except for the docrep license question. waiting for answer from author.

@amnona
Copy link
Collaborator Author

amnona commented Feb 19, 2018

ok, fixed the kwargs and moved notebooks to /docs/source/notebooks/
currently still pip installing docrep (not copied to calour repo).
all comments should be addressed.

@RNAer RNAer merged commit 83f9aad into biocore:master Feb 20, 2018
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.

None yet

2 participants