Update installation instructions #154
Conversation
Codecov Report
@@ Coverage Diff @@
## main #154 +/- ##
=======================================
Coverage 92.13% 92.13%
=======================================
Files 48 48
Lines 3294 3294
=======================================
Hits 3035 3035
Misses 259 259
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
docs/source/contributing.rst
Outdated
|
|
||
|
|
||
| Alternatively: | ||
|
|
There was a problem hiding this comment.
It's not entirely "alternatively" because the docs requirements also have to be installed for the previous approach.
docs/source/contributing.rst
Outdated
| Alternatively: | ||
|
|
||
| 1. Navigate to the docs directory ``docs/`` | ||
|
|
There was a problem hiding this comment.
I've been changing this phrasing to "Change directory to ..." since "Navigate" seems to imply the use of a file manager, but we're referring here to the use of a terminal.
There was a problem hiding this comment.
Also changed other occurrences of "Navigate" to "Change directory" .
docs/source/examples.rst
Outdated
| @@ -6,6 +6,19 @@ Usage Examples | |||
| .. toctree:: | |||
| :maxdepth: 1 | |||
|
|
|||
| .. _example_dependencies: | |||
| Example Dependencies | |||
| ------------------------ | |||
docs/source/examples.rst
Outdated
| Example Dependencies | ||
| ------------------------ | ||
|
|
||
| Some examples may use additional dependecies. These dependencies are listed in `examples_requirements.txt <https://github.com/lanl/scico/blob/main/examples/examples_requirements.txt>`_. |
There was a problem hiding this comment.
The "may" is redundant given the presence of the "some" - I suggest removing it. Also, there's a typo in "dependecies", and perhaps change "... dependecies. These are ..." to "... additional dependencies, which are ...".
docs/source/examples.rst
Outdated
| ------------------------ | ||
|
|
||
| Some examples may use additional dependecies. These dependencies are listed in `examples_requirements.txt <https://github.com/lanl/scico/blob/main/examples/examples_requirements.txt>`_. | ||
| Pip should be used to install these extra requirements except astra which should be installed via conda: |
There was a problem hiding this comment.
I suggest a minor rephrasing to:
The additional requirements should be installed via pip [or perhaps pip?], with the exception of astra [or astra-toolbox?], which should be installed via conda:
docs/source/examples.rst
Outdated
| conda install -c astra-toolbox astra-toolbox | ||
| pip install -r examples/examples_requirements.txt # Installs other example requirements | ||
|
|
||
| The dependencies can also be installed one by one as required. |
There was a problem hiding this comment.
Perhaps replace "one by one" with "individually"?
docs/source/examples.rst
Outdated
| ------------------------ | ||
|
|
||
| Some examples may use additional dependecies. These dependencies are listed in `examples_requirements.txt <https://github.com/lanl/scico/blob/main/examples/examples_requirements.txt>`_. | ||
| Pip should be used to install these extra requirements except astra which should be installed via conda: |
There was a problem hiding this comment.
It looks to me like this PR removes any notion of ASTRA as a dependency for SCICO. I'm uncomfortable with this because (1) ASTRA is needed to run some of the tests and (2) ASTRA-based LinOps are in the API reference without any note saying you must install ASTRA for them to work.
Is there some place where we clarify (even for ourselves) what should work (examples, tests, some other thing) after the user follows which set of install instructions?
There was a problem hiding this comment.
Agreed, this should be handled better. See also #155.
There was a problem hiding this comment.
We should have a discussion sometime on issue #155.
In the meantime, should I add a sentence in the contributing section stating that installing the example_requirements is necessary for successfully running the tests?
There was a problem hiding this comment.
@smajee That, or just conda, given that (I think) it's the only one needed to run the tests. See also my comment about the github action that runs the tests.
There was a problem hiding this comment.
Added a comment in commit 617e993
There was a problem hiding this comment.
Please check that the dev install instructions are reasonably in sync with the commands in https://github.com/lanl/scico/blob/main/.github/workflows/pytest.yml (specifically, the "Install dependencies" step)
…nto smajee/install_instructions
They seem to match except libopenblas-dev and pytest-cov. |
Address nitpicks.
| pip install -e . # Installs SCICO from the current directory in editable mode | ||
|
|
||
|
|
||
| For installing dependencies related to the examples please see :ref:`example_notebooks`. | ||
| Installing these are neccessary for the successfull running of the tests. |
There was a problem hiding this comment.
This seems easy to miss to me, but I think it's fine for now given that we will likely look at all of this again for #155 .
Fixes issue #139