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

Update core analysis docs #278

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Conversation

cbethell
Copy link
Contributor

@cbethell cbethell commented Jan 6, 2023

Issue Addressed
Closes #275, #274, and #263

What is the purpose of these changes?

This PR makes the recommended updates to the main README.md file in preparation for further usability evals.

What changes did you make?

The following changes are made in this PR:

Any comments, concerns, or questions important for reviewers

Do these changes seem to adequately address the issues mentioned/tagged above?

Checklist

Place an x in all boxes that you have completed.

  • I have run the most recent version of the code
  • If I am adding in reports or generating any plots, I have reviewed the necessary reports
  • I have added all necessary documentation (if applicable)

- number sections
- note min version of Snakemake
- emphasize places in example commands that would need to be replaced
@cbethell cbethell requested a review from allyhawkins January 6, 2023 21:00
Copy link
Member

@allyhawkins allyhawkins left a comment

Choose a reason for hiding this comment

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

I think rather than changing to the all caps, underscore separated paths in the examples, we should include notes about what the individual arguments represent and then a reminder to replace the paths in bold immediately under the command. I think by explicitly stating it we might get through to more people and we should be sure to explain what each option actually represents.

README.md Outdated
Comment on lines 66 to 67
--config results_dir="<REPLACE_WITH_RELATIVE_PATH_TO_RESULTS_DIRECTORY>" \
project_metadata="<REPLACE_WITH_RELATIVE_PATH_TO_YOUR_PROJECT_METADATA_TSV>"
Copy link
Member

Choose a reason for hiding this comment

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

I actually think I would remove the underscores in between the words. I'm also not crazy about the all caps, but maybe that will draw people's attention to it.

I also think you want to add a few sentences below that state: "Where config_results_dir is the relative path to the results directory where all results from running the workflow will be stored". And then do the same for the project metadata.
We do something similar in scpca-nf - https://github.com/AlexsLemonade/scpca-nf/blob/main/external-instructions.md#configuring-scpca-nf-for-your-environment

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@cbethell
Copy link
Contributor Author

I've implemented your suggestions to add a line or two explaining the parameters and bolding/emphasizing the need to replace with the relevant filepaths @allyhawkins -- I did keep the CAPS since that was an original suggestion from Deepa but agree that the underscores were much so I removed them. Let me know what you think now!

@cbethell cbethell requested a review from allyhawkins January 10, 2023 19:12
Copy link
Member

@allyhawkins allyhawkins left a comment

Choose a reason for hiding this comment

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

This looks good! Just one sentence order switch, but no need for me to look at it again.

README.md Outdated Show resolved Hide resolved
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.

2 participants