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

feat: Add Sphinx Documentation #25

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

feat: Add Sphinx Documentation #25

wants to merge 16 commits into from

Conversation

coolalexzb
Copy link
Contributor

This PR aims to add Sphinx documentation for pyhf-benchmark.
The main page of documentation has been completed, and the page looks like as follows:

截屏2020-07-27 下午10 24 32

More contents need to be added into the introduction, examples, etc.

@matthewfeickert matthewfeickert added the documentation Improvements or additions to documentation label Jul 28, 2020
Copy link
Member

@matthewfeickert matthewfeickert 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 great to see. I'll enable some extensions that will help with testing Sphinx builds.

# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
extensions = ["m2r2"]
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now, but given that m2r2 is a fork of an abandoned project it is probably worth just switching to RST for long term support, which pyhf reluctantly did. That can be for another PR though. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, since I wanted to use README.md directly rather than write README.rst before. I will discard the usage of m2r2 and wrote a README.rst file in the next PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah keeping it as Markdown I think is fine for now.

@matthewfeickert
Copy link
Member

For Sphinx to really be powerful it will need to be able to understand the code though, so it will help to get PR #19 done first.

@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #25 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #25   +/-   ##
=======================================
  Coverage   85.89%   85.89%           
=======================================
  Files          12       12           
  Lines         482      482           
  Branches       67       67           
=======================================
  Hits          414      414           
  Misses         54       54           
  Partials       14       14           
Flag Coverage Δ
#unittests 85.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf_benchmark/jsonlfile.py 96.36% <ø> (ø)
src/pyhf_benchmark/load.py 100.00% <ø> (ø)
src/pyhf_benchmark/manager.py 100.00% <ø> (ø)
src/pyhf_benchmark/mle.py 100.00% <ø> (ø)
src/pyhf_benchmark/plot.py 89.87% <ø> (ø)
src/pyhf_benchmark/run.py 83.90% <ø> (ø)
src/pyhf_benchmark/stats.py 66.91% <ø> (ø)
src/pyhf_benchmark/util.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc0f912...4883ed0. Read the comment docs.

@coolalexzb
Copy link
Contributor Author

coolalexzb commented Aug 14, 2020

@matthewfeickert Documentation contents have been added and you can enter into docs directory and use sphinx-build -b html source build command to look through the documentation contents.
截屏2020-08-14 下午4 59 58
截屏2020-08-14 下午4 59 54

@matthewfeickert matthewfeickert marked this pull request as draft August 15, 2020 00:36
@coolalexzb
Copy link
Contributor Author

@matthewfeickert What are the further steps for this PR?

@coolalexzb coolalexzb marked this pull request as ready for review August 17, 2020 19:53
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

I need to preview this still and get the ReadTheDocs integration setup but this is looking good overall. Some things need fixing though.

xref_links = {"arXiv:1007.1727": ("[1007.1727]", "https://arxiv.org/abs/1007.1727")}

# Github repo
issues_github_path = "scikit-hep/pyhf/pyhf-benchmark"
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
issues_github_path = "scikit-hep/pyhf/pyhf-benchmark"
issues_github_path = "pyhf/pyhf-benchmark"

path is wrong

docs/source/development.rst Outdated Show resolved Hide resolved
Comment on lines 29 to 31

Returns:

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
Returns:

If no return don't list it.

Comment on lines 141 to 142
Returns:

Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned before, probably makes more sense to have the function accept an axis object and then return an axis object then try to wrap all of it, but that can be done for a later PR. At the moment this doesn't return anything so it should get fixed.


Returns:

"""
fig, ax = plt.subplots()
ax.set_xlabel("Time (minutes)")
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be seconds not minutes.

@matthewfeickert
Copy link
Member

None of the required dependencies are setup in a docs extra in setup.py so there is no way to build the docs at the moment if you were a developer who just installed everything. All the requirements need to be listed.

@coolalexzb
Copy link
Contributor Author

coolalexzb commented Aug 19, 2020

@matthewfeickert I created a virtual python environment without any other third-part library installed before successfully, but it is weird that when I use sphinx-build -b html source build command, I can build the docs successfully without any notification that I need to install other libraries.

I changed README.md to README.rst and removed m2r2 extension in conf.py. I think you can try to build the docs again to see if there is any other problem.

@coolalexzb
Copy link
Contributor Author

@matthewfeickert One question in command line api page, the format of command lines in examples are in a mess, can you tell me how to modify it?
截屏2020-08-19 下午5 24 26

@coolalexzb
Copy link
Contributor Author

@matthewfeickert The bug in CI/docs (3.8) still needs to be solved. What is the possible reason?

@matthewfeickert
Copy link
Member

matthewfeickert commented Aug 20, 2020

I created a virtual python environment without any other third-part library installed before successfully, but it is weird that when I use sphinx-build -b html source build command, I can build the docs successfully without any notification that I need to install other libraries.

It seems that you don't actually have a properly isolated virtual environment. If on this branch in a clean virtual environment I get the following

$ python -m pip install --upgrade -e .[complete]
$ python -m pip list | grep "Sphinx\|sphinx"
Sphinx                        3.2.1
sphinxcontrib-applehelp       1.0.2
sphinxcontrib-devhelp         1.0.2
sphinxcontrib-htmlhelp        1.0.3
sphinxcontrib-jsmath          1.0.1
sphinxcontrib-qthelp          1.0.3
sphinxcontrib-serializinghtml 1.1.4
$ cd docs
$ make html
Running Sphinx v3.2.1

Extension error:
Could not import extension sphinxcontrib.bibtex (exception: No module named 'sphinxcontrib.bibtex')
make: *** [Makefile:21: html] Error 2

The bug in CI/docs (3.8) still needs to be solved. What is the possible reason?

If you look at the logs

Run python setup.py build_sphinx
  python setup.py build_sphinx
  touch docs/_build/html/.nojekyll
  shell: /bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.8.5/x64
running build_sphinx

Application error:
config directory doesn't contain a conf.py file (docs)
##[error]Process completed with exit code 1.

it tells us the same error as

$ python setup.py build_sphinx
running build_sphinx

Application error:
config directory doesn't contain a conf.py file (docs)

as conf.py isn't at the top level of docs/

$ find . -iname "conf.py"
./docs/source/conf.py

. Why are all the docs under source/? Is this a new recommendation from Sphinx? The first error from make html isn't a surprise though as you have have required the module

$ git grep "bibtex"
docs/source/conf.py:    "sphinxcontrib.bibtex",

but haven't added it to the docs extra

$ git diff origin/master -- setup.py
diff --git a/setup.py b/setup.py
index c8df5b1..1d944ad 100644
--- a/setup.py
+++ b/setup.py
@@ -20,6 +20,7 @@ extras_require["develop"] = sorted(
         + ["check-manifest", "bumpversion~=0.5", "pre-commit", "twine",]
     )
 )
+extras_require["docs"] = sorted(set(["sphinx"]))
 extras_require["complete"] = sorted(set(sum(extras_require.values(), [])))
 
 setuptools.setup(extras_require=extras_require,)

@coolalexzb
Copy link
Contributor Author

@matthewfeickert I use the same command lines as you did but I can still build the doc successfully even I denote a python version as 3.8(which is a different version with the python in my local PC) in virtual environment. As the information from your result. I just need to add to setup.py?
extras_require["docs"] = sorted(set(["sphinxcontrib.bibtex"]))

@coolalexzb
Copy link
Contributor Author

@matthewfeickert If I don't use source file I will get an error when build docs locally. The error information is the same as CI/docs (3.8)
截屏2020-08-19 下午11 54 37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants