min updates - Githubissues

JMichaelStringer commented 3 weeks ago

now with CRLF endings fixed to LF, run_all_steps.sh completes with outputs as expected.

JMichaelStringer commented 2 weeks ago

@suzytamang @dax-westerman @esther-meerwijk

please review and merge, so I can update systems with the new code and outputs.

esther-meerwijk commented 2 weeks ago

Overall these changes look okay to me, but I do have a few requests/questions:

grabtargets is writing output files to the ./res/ folder. That folder is meant for resources, other than code, that serve as input for the process. Is there a more logical location to write those output files?
Under the ./tests folder there is a /resources folder that has code. To be consistent with the rest of clever-rockies I would rename /resources to /src and rename test_notes/ to res/. I have more questions about files related to the test notes but that is separate from this PR.
In na_trigs.json RISK was added for LONELINESS and it sounded like this was done to satisfy the test notes. The NA triggers and NEG triggers should be based on what SMEs say they should be. If that leads to a test not passing we should (and I did) change the test. I think these trigger files should be the same as on Synapse.

esther-meerwijk commented 2 weeks ago

FYI, for our research projects we're using this folder structure to keep things organized:

src: any code that is executed. .py, .R, *.sql files etc. (src is an abbreviation of source, as in source code).
run: any output that is produced by executing code in ./src.
res: any files that serve as input when executing code in ./src. res is short for resources.
etc: any files we want to keep that do not fit the prior 3 folders. Files in ./etc are not needed to execute code in ./src and are not typically added to git. etc is short for, you guessed it, et cetera.

JMichaelStringer commented 2 weeks ago

@esther-meerwijk updated to address comments

suzytamang / clever-rockies