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

online documentation #137

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

dennisbrookner
Copy link
Member

It finally happened, I went rogue and built docs for careless! No rush to get this merged in, and theres's still a little work to be done, as outlined below.

Changes in this PR

  • I added a docs/ folder containing fairly boilerplate conf.py, MAKEFILE, and make.bat files, along with:
    • index.md, the contents of which are grabbed from the repo README
    • careless.md, documenting the CLI for the core careless function
    • stats.md, documenting all other command-line utilities included in careless
      • All except careless.filter_by_image_cc, which didn't work for reasons that I don't understand.
    • double_wilson.md, a description of the double Wilson algorithm that was already present in the repo.
  • IMPORTANT: small changes to the code base were made in order to allow building documentation through autoprogram. See the issue I raised there for details:
  • I added workflows test_docs.yml and build_docs.yml. The former runs on any pull request to main and checks that the website builds; the latter runs on any push to main, and builds and deploys the website
  • Migrated setup.py to pyproject.toml. I originally intended to not do this, but as I got deeper into these changes, it became the path of least resistance
    • There are now .[docs] and .[test] installation options
    • setup.py-based installation is now no longer supported 😢
    • build.yml and publish.yml were updated accordingly.
  • updated links to preprint to refer to the nature comms paper

Action items before merging

  • Let me know if there are any changes you'd like to the website. The site is live at https://dennisbrookner.github.io/careless/index.html, so it's easiest to just inspect it there! I'm happy to change/add anything
    • Do you want something other than a copy of the README on the homepage?
    • Do you want to keep the double Wilson page? Do you want any other pages of that type?
    • Should the stats functions be split across more different pages, akin to how the rs-booster docs are structured?
  • Figure out why building documentation for careless.filter_by_image_cc gives a weird segfault. Or don't, I guess. I don't know how critical this is.
  • Make sure nothing critical was lost in the setup.py --> pyproject.toml transition, and if it was, add it back in. For example:
    • LONG_DESCRIPTION
    • I don't know what the following are/do, but they should get re-added:
    scripts=[
        "scripts/make_difference_map",
        "scripts/stream2mtz",
    ],
  • confirm that my code changes were lossless, as I suspect they are. They change this:
def main():
    parser = ArgumentParser().parse_args()
    run_analysis(parser)

to this:

def parse_arguments():
    return ArgumentParser()

def main():
    run_analysis(parse_arguments().parse_args())

so that the parse_arguments() function can be understood by the documentation parser.

  • Change references to my fork to refer back to the main repo. This will temporarily break things, but make the merge smoother.

Action items after merging

  • Make sure that the new publish.yml script works!! This sadly can't be tested on my fork.
  • Link to the docs on the rs-station website
  • Go through the little dance of creating an empty, unlinked gh-pages branch. I think this is necessary to do once and won't happen automatically
    • Add an empty .nojekyll file to the gh-pages branch, so that CSS and such gets read properly

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2023

Codecov Report

Merging #137 (2d07ad9) into main (4f25d47) will increase coverage by 0.03%.
Report is 1 commits behind head on main.
The diff coverage is 37.14%.

@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
+ Coverage   79.09%   79.12%   +0.03%     
==========================================
  Files          52       52              
  Lines        2353     2362       +9     
==========================================
+ Hits         1861     1869       +8     
- Misses        492      493       +1     
Flag Coverage Δ
unittests 79.12% <37.14%> (+0.03%) ⬆️

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

Files Changed Coverage Δ
careless/stats/prior_b.py 0.00% <0.00%> (ø)
careless/stats/rescale.py 0.00% <0.00%> (ø)
careless/stats/completeness.py 87.87% <33.33%> (+0.37%) ⬆️
careless/stats/filter_by_image_cc.py 89.55% <33.33%> (+0.15%) ⬆️
careless/stats/rsplit.py 90.41% <33.33%> (+0.13%) ⬆️
careless/stats/ccanom.py 88.75% <50.00%> (+0.14%) ⬆️
careless/stats/cchalf.py 91.02% <50.00%> (+0.11%) ⬆️
careless/stats/ccpred.py 92.00% <50.00%> (+0.10%) ⬆️
careless/stats/history.py 89.65% <50.00%> (+3.44%) ⬆️
careless/stats/image_cc.py 89.83% <50.00%> (+0.17%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

setup.py Outdated Show resolved Hide resolved
@kmdalton
Copy link
Member

This looks like a great start. It'll take me some time to review. Let me address some of these action items:

  • The website looks good. I'll have to stare for a bit to decide on specific requests.
    • I think the README is an okay starting place. In my dream world, the tf installation instructions would be automatically pulled into the page.
    • The double Wilson page was mostly for my notes / records, but it does not hurt to keep it. That being said, some of the tex formatting seems broken which it would be nice to fix. Also maybe we should rename the page to the "Double Wilson" prior rather than algorithm.
    • I think it would be helpful to break this down into more sections, yes. I am happy to advise on the specific section names, but beforehand I just wanted to note that a couple of the console scripts are missing. What happened to careless.image_cc and careless.xds2mtz ?
  • scripts /make_difference_map and scripts/stream2mtz are old programs that are probably still referenced in careless-examples. I think they should be removed. People should use rs-booster for difference maps. I think stream2mtz can just go away wholesale, because careless just supports .stream files now. Feel free to remove this stuff. In general, the scripts directory is really just a dumping ground for code snippets that probably should be in rs-booster or the stats / io submodules. I'd be comfortable just deleting the whole directory and sorting it out later.
  • As far as I can tell, your changes should be safe. The CI still passes which is a good sign.

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.

3 participants