Skip to content

[query] Weighted linear regression rows#10632

Merged
danking merged 29 commits into
hail-is:mainfrom
johnc1231:johnc-linreg-weights-experiment
Jul 8, 2021
Merged

[query] Weighted linear regression rows#10632
danking merged 29 commits into
hail-is:mainfrom
johnc1231:johnc-linreg-weights-experiment

Conversation

@johnc1231

Copy link
Copy Markdown
Contributor

No description provided.

@tpoterba

tpoterba commented Jul 1, 2021

Copy link
Copy Markdown
Contributor

sweet!

@patrick-schultz patrick-schultz left a comment

Copy link
Copy Markdown
Member

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, though I didn't think very hard about the quality of the IR being generated. I do wonder if it might be worth trying to avoid the multiplications by 1 in the unweighted case, but that could be a future optimization.

// )((accum, v) => accum.concat(v.toS).concat(" "))
// .concat("] vs [ ")
// )((accum, v) => accum.concat(v.toS).concat(" "))
// .concat(s"] and operation ${Pretty(body)}"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug

@johnc1231

Copy link
Copy Markdown
Contributor Author

I can remove multiplications by 1 in a subsequent PR. I think not hard and probably not much of a difference. At the moment, the only person using this method will be using it to enable weighted linreg anyway, so not super worried.

patrick-schultz
patrick-schultz previously approved these changes Jul 1, 2021
@johnc1231 johnc1231 added the WIP label Jul 1, 2021
@johnc1231

Copy link
Copy Markdown
Contributor Author

Something is wrong with the annotate_globals. It's taking way too long when used on something modestly large. I think it's redoing work somewhere.

@tpoterba

tpoterba commented Jul 7, 2021

Copy link
Copy Markdown
Contributor

Reassigning to me since Patrick is OOO this week

@johnc1231 johnc1231 removed the WIP label Jul 7, 2021
@johnc1231

johnc1231 commented Jul 7, 2021

Copy link
Copy Markdown
Contributor Author

Great, thanks. Should be ready to be reviewed now, the performance is now where I want it to be. (18x faster than aggregators for Caitlin's use case)

@tpoterba tpoterba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! One question.

Comment thread hail/python/hail/methods/statgen.py Outdated
@johnc1231

Copy link
Copy Markdown
Contributor Author

Also made it so that y and cov don't get scaled when weights is None, and then sqrt_weights can just be missing.

@danking danking merged commit 06b81b7 into hail-is:main Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants