-
Notifications
You must be signed in to change notification settings - Fork 4
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
Document package scope in README #11
Conversation
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.
Thanks for adding this!
In particular it would be helpful to get feedback on whether the scope is clearly explained. We don't want to document every part of the pipeline here, but there should be enough basic detail that we can use the readme as a reference on scope.
Makes sense that you don't want to document every part of the pipeline here. As I wrote in comments, I think it could be clearer which functionality is provided by the package and which is outside the package. As an example:
Model fits are saved as flat files with standardized formats and RDS objects.
This package doesn't implement saveRDS
. Model fits being saved is a step in the pipeline, not a contribution to that step by this package. I'd guess the contribution to this step by the package is that it modifys all outputs to be in some standardised format. If that's the bulk of the job being done by the package at this step I think that should be brought out more.
I wonder if there is some place where we have documentation of the whole package pipeline that could be linked to from the README, then it could be easier to say these are the parts which the package provides functionality. But maybe there is no public joint overall pipeline documentation?
That doesn't exist...yet. If you're interested, that would be helpful! |
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.
I think this will be super helpful
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.
Thanks for the updates! Just suggest maintaining the {EpiNow2}
convention (if we're going to use it at all). Personally I'd go further with focusing on the role of the package separate to the pipeline, but I appreciate it's a small point so I'm happy to get things moving and come back to this once we have more functions in the package.
Co-authored-by: Adam Howes <[email protected]>
8783a18
to
c90f5a4
Compare
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
Set up the readme with some description of what we're doing here and what we're not doing. I was aiming to explain the purpose generally enough that we could point an interested non-CFA partner to this repo. I also tried to add enough specificity to keep the scope small and clear.
In particular it would be helpful to get feedback on whether the scope is clearly explained. We don't want to document every part of the pipeline here, but there should be enough basic detail that we can use the readme as a reference on scope.
If something is still unclear or the wording is a little lax, suggested edits are appreciated!