saige_assoc.py review

[katiedelange]

Some points to consider in saige_assoc.py

• Not all of the SAIGE parameters are self-explanatory, and I can't find explanations for everything in the docs yet. Could you add some details to the README for anything that we might expect to have to vary (e.g. what is a reasonable value range for tol and how might I go about selecting that, or am I never going to want to change it?)

• I assume this subsetting needs to be removed eventually

[annacuomo]

Not all of the SAIGE parameters are self-explanatory, and I can't find explanations for everything in the docs yet. Could you add some details to the README for anything that we might expect to have to vary (e.g. what is a reasonable value range for tol and how might I go about selecting that, or am I never going to want to change it?)

This is an excellent point, I'll add some of these in the SAIGE-QTL docs but then also in the readme here (PR: https://github.com/populationgenomics/saige-tenk10k/pull/75)!

Also as a related aside I just realised that most of these are listed in the code but not actually customisable! I added a couple for debugging purposes recently (https://github.com/populationgenomics/saige-tenk10k/blob/real-association/saige_assoc.py#L355-L356), but I think this is not ideal.

I think lint might complain if I add seven million arguments / paraments with click, so maybe I should add some sort of config file instead? I've added this to my TO DO list.

[annacuomo]

I assume this subsetting needs to be removed eventually

@katiedelange yup this is just for testing :)

[annacuomo]

@katiedelange back to checking these issues you left a while back! I think this one can be closed now as your main points have been addressed?

• SAIGE-QTL flags are described in the README and most pars have been moved to the config file making it easier to customise leaving the script lighter
• and the subsetting has been removed?

let me know if you have any other comments