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

Documentation read through #219

Merged
merged 51 commits into from
Dec 3, 2024
Merged

Conversation

raphaelshirley
Copy link
Member

We are working through installing according to the documentation and making changes as we find them.

Closes #216

Code Quality

  • I have read the Contribution Guide
  • My code follows the code style of this project
  • My code builds (or compiles) cleanly without any errors or warnings
  • My code contains relevant comments and necessary documentation

Project-Specific Pull Request Checklists

Bug Fix Checklist

  • My fix includes a new test that breaks as a result of the bug (if possible)
  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

New Feature Checklist

  • I have added or updated the docstrings associated with my feature using the NumPy docstring format
  • I have updated the tutorial to highlight my new feature (if appropriate)
  • I have added unit/End-to-End (E2E) test cases to cover my new feature
  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

Documentation Change Checklist

Build/CI Change Checklist

  • If required or optional dependencies have changed (including version numbers), I have updated the README to reflect this
  • If this is a new CI setup, I have added the associated badge to the README

Other Change Checklist

  • Any new or updated docstrings use the NumPy docstring format.
  • I have updated the tutorial to highlight my new feature (if appropriate)
  • I have added unit/End-to-End (E2E) test cases to cover any changes
  • My change includes a breaking change
    • My change includes backwards compatibility and deprecation warnings (if possible)

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.95%. Comparing base (0fd8408) to head (f4c913f).
Report is 71 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Raphael Shirley and others added 20 commits October 15, 2024 13:44
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
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
Raphael Shirley and others added 24 commits November 12, 2024 17:38
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
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
@raphaelshirley
Copy link
Member Author

@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.

Copy link
Member

@johannct johannct left a 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.
@raphaelshirley raphaelshirley merged commit f0a9967 into main Dec 3, 2024
14 checks passed
@raphaelshirley raphaelshirley deleted the issue/216/documentation-read-through branch December 3, 2024 12:59
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.

Documentation read through
3 participants