search

mgalardini / pyseer

SEER, reimplemented in python 🐍🔮
http://pyseer.readthedocs.io
Apache License 2.0
104 stars 25 forks source link

Fix summarize_annotations when using elastic net #126

Closed lvclark closed 3 years ago

lvclark commented 3 years ago

Using output from the elastic net model, I was getting an error because no p-value correcting for population structure was reported. This change allows summarize_annotations to use the p-value that doesn't correct for population structure in that case.

mgalardini commented 3 years ago

Hi, thanks for reporting this issue and a potential solution. Maybe it would make more sense for the script to have an option to consider the filtering p-value instead of the lrt one. This way it is more explicit which pvalue is actually used and can avoid confusion. Does that make sense?

johnlees commented 3 years ago

@mgalardini Should we turn on travis on branches/PRs? I guess it would be useful to see the tests here (I also just noticed I had broken them with a recent commit, but have fixed again now)

mgalardini commented 3 years ago

Sure, that makes sense, let me do it then

mgalardini commented 3 years ago

I'm not sure why it does not show up here, but Travis is running tests for PRs: https://travis-ci.org/github/mgalardini/pyseer/builds/741672174

johnlees commented 3 years ago

Quick search suggests the solution is to remove and add again -- a classic

@lvclark Would you be able to merge master into this to pull in the test fix?

mgalardini commented 3 years ago

Ah, of course. I did something similar, but unsure if that works. Will check later again

lvclark commented 3 years ago

@johnlees Ok, I've done the merge!

@mgalardini Adding an option to the script makes sense. I wasn't sure if you'd actually want to merge this PR, but I wanted to at least bring the issue to your attention!

johnlees commented 3 years ago

I'd suggest that we merge this, and then add the script option. I can do this shortly