-
Notifications
You must be signed in to change notification settings - Fork 83
Make it such that sample distribution scripts can be run from anywhere in project #79
Conversation
Changed the .spelling comment for consistency
Because the paths are hardcoded
With 0a26f6a, I've taken out |
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.
Took a gander at how you were doing this. Nifty approach! It seems like we could point folks at this to ease their use during development. Not sure if you wanted this reviewed yet, so I just markedo ne thing I noticed as I looked at how this was done.
@@ -1,2 +1,4 @@ | |||
histology-pie_files | |||
histology-treemap_files | |||
# The nonessential output produced in the root directory of this repository when we run `analyses/sample-distribution-analysis/run-sample-distribution.sh` |
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.
# The nonessential output produced in the root directory of this repository when we run `analyses/sample-distribution-analysis/run-sample-distribution.sh` | |
# The nonessential output produced in the root directory of this repository when we run `run-sample-distribution.sh` |
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 that since it's in the same directory, the path should only be the script name.
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.
Good idea -- added in 148f376
This is ready for review. |
From CircleCI output:
What I've read suggested that is due to the presence of comments within the RUN statements, so I moved them in dcd9b23 while I was at it. |
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.
This looks good to me, but I'd appreciate a pair of eyes that are more familiar with R on it. @cbethell or @cansav09 - could one of you take a look this morning too?
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.
My comments may be me misunderstanding how to use the changes made to this script.
@@ -5,7 +5,7 @@ | |||
To run the scripts in this module from the command line, use: | |||
|
|||
``` | |||
bash "analyses/sample-distribution-analysis/run-sample-distribution.sh" | |||
bash run-sample-distribution.sh |
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.
Is this assuming you have set your current directory as the sample-distribution-analysis
?
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.
With these new additions can we have a sentence or two here that clarifies what this script will or won't assume?
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.
If you are in the directory with the README, you are in the directory where the bash script is and yes I can say that the script will always run as if you are calling it from the directory it lives in. The purpose of the changes are so these scripts will run correctly no matter what your working directory is when you call it.
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.
Cool. Then my running of the scripts were appropriate tests then. Just wanted to check that everything it was and wasn't doing was what was expected. Worked for me!
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!
Purpose/implementation
This is the possibly overly complicated thing mentioned #78 (review). It occurred to me that someone might navigate to the sample distribution directory and try to run the Rscripts, i.e.:
cd analyses/sample-distribution-analyses Rscript 01-filter-across-types.R
This would raise an error because this is supposed to be run from the root of the project and the data, results, etc. directories are hardcoded as such.
This prompted me to try a strategy like what we have in lines 3-8 of
./scripts/run_in_ci.sh
@cbethell you were onto something when you mentioned
setwd()
in person on Wednesday. Using it is generally considered a bad idea for portability.By adding the changes I have to the shell and R scripts in this module it ensures:
--vanilla
addition is probably more important if this is run outside the Docker container.rprojroot::find_root(rprojroot::has_dir(".git"))
to find the root of the project. The criterion is where the.git
folder lives. (The irony of using this in conjunction withsetwd()
is not lost on me.)Issue
N/A - I don't think this should be a requirement or included as part of the instructions for #77. I was curious and wanted to see if it worked given how this project structure is a bit different from how I usually set up my own projects.
Directions for reviewers
If you are willing to check out my branch, I would try running these scripts using different places as your working directory.
Docker and continuous integration