-
Notifications
You must be signed in to change notification settings - Fork 1
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
Documentation read through #219
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #219 +/- ##
==========================================
+ Coverage 71.93% 71.95% +0.01%
==========================================
Files 50 50
Lines 6432 6435 +3
Branches 937 937
==========================================
+ Hits 4627 4630 +3
Misses 1805 1805 ☔ View full report in Codecov by Sentry. |
There was a repeated line with a typo which did not do anything so I removed it.
Following testing by Olivier we realised it was easy to have a notebook kernel that was not inside the correct environment. We decided that this was a common enough mistake that we should add explicit instructions tot eh documentation
Since including the pybind11 code using the MANIFEST.in file I could install on my mac from source using pip. I am concerned that this wouldn't work if I did not already have compilers and libraries present so I recommned people follow the developer install instructions which install the required libraries if there is no binary available
A number of changes suggested in the PR. Some section renames and addition of extra lines to code snippets for clarity
…f version into the rtf
…/216/documentation-read-through
Some of the change requests were ignored as they are required for the notebook to run. I have so far left in the onesource examples at the end as they were previously requested. We can discuss if to remove them this week. The issue with curl to download the para file was that it needed the url updating to the raw file not the html. The example para file does not run through the downloader because it needs the sed list links to be relative. We can either update the para file or the downloader to append these links
He requested a couple of changes which will increase run time. We need to check that all notebooks still complete in time for the documentation. I also added a simple z-z plot
There are various small changes and cleaning things up. We have to set up the environment variables but given the new structure a user may also need a further environment variable to reference the executables but I will not mention that for now for simplicity.
Mainly adding git clone code for clarity following Olivier sugestion
I also added a custom css to make the tables wrap so be easier to read Many changes to keyword descriptions including removing the defunct keywords
Mainly installation instructions
We have moved the developer install to a new section and reordered gettign started
We wanted to be more informative regarding the contruction of the input tables which have a strict format
simplified and retured the standard filters and removed low level functionality section
We have run through the first main notebook examples and called other developer examples
calzetti.dat not present so changed to SB_calzetti.dat
These are from changes to process. I need to check the notebooks are still working
Also deleted the seperate notebooks page as it is the core of the Python interface page which now has all teh notebooks
… issue/216/documentation-read-through
When this documentation is merged we will define v0.2.0 and deprecate the GitLab version
hub is requireing a custom user now to prevent getting spammed. see readthedocs/readthedocs.org#11763
It seems for docs to build we need to add a user but this broke the test so I have removed the test to see if it will tehn build
I had to send a specific downloader to check the call
@olivierilbert @johannct What do you think about merging this now? There is a lot of work in there and we can start a new branch for ongoing minor changes perhaps. I am also keen to check if my fix makes it build on readthedocs again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. There is just one warning : src/lephare/data_retrieval.py#L267 added line not covered by tests. If you can add a test, it is great.
In order to get buildthedocs running we needed to add a user agent to the pooch downloader. this in turn introduced the specification of a downloader We added a very short test just to call the code without a downloader specified and check it runs.
We are working through installing according to the documentation and making changes as we find them.
Closes #216
Code Quality
Project-Specific Pull Request Checklists
Bug Fix Checklist
New Feature Checklist
Documentation Change Checklist
Build/CI Change Checklist
Other Change Checklist