Repo reorganization #207

mike-w-wilson · 2020-03-25T22:22:41Z

Summary

This PR is pretty massive but since it is a reorganization of the entire repo, I couldn't think of another way to do it. This organization was drafted and previously discussed here:

https://docs.google.com/spreadsheets/d/1pVpru0acCCr1AvSRyf89X-LxPKhwK-mYCBtZvICl9fQ/edit#gid=0

The changes include import statements, loggers, and where functions live. Six functions have also been removed as they are built into hail or unnecessary for other reasons. They are:

gnomad.utils.generic.reverse_complement_bases(bases)
gnomad.utils.generic.get_sample_data(mt, …)
gnomad.utils.generic.phase_by_transmission(…)
gnomad.utils.generic.phase_trio_matrix_by_transmission(tm)
gnomad.utils.generic.explode_trio_matrix(tm)
gnomad.utils.generic.sample_pcs_uniformly(…)
gnomad.utils.liftover.main(args)

Once this is approved, I can run Black.

nawatts

👍 based on the spreadsheet.

For the __init__.py files, we might consider importing the submodules themselves instead of everything in them to avoid having so many things all in the same namespace. Or using __all__ to define what's exported.

https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

- removed field_utils.py (functions were moved elsewhere) - moved vcf-related functionalities to vcf.py (I envision this will become the place for all VCF needs)

lfrancioli

Thanks a bunch @mike-w-wilson for getting this going, looks great!
A few things:

I committed a few changes myself -- please take a look at that last commit
I'd like to move pop colors to ancestry.py; thoughts? Also ancestry.py vs populations.py ?
How about moving random_forest.pretty_print_runs to the gnomad_qc repo? Seems very specialized and possibly not used in other projects. @jkgoodrich, did you use it for UKBB ?
should we rename rf.py to random_forest.py ? it'll break some code and I'm unsure it really makes a difference But happy to adapt if consensus is that it's desirable.

mike-w-wilson

Just a couple comments but this looks good to me. The moves from field_utils to annotations make sense.
Putting pop colors in ancestry is a good idea and seems like it would be used. I don't have strong feelings on ancestry.py vs. populations.py. I think we discussed this in one meeting but I'll let @jkgoodrich and @gtiao weigh in.
Fine with me if it is okay with Julia.
I went back and forth on this. I spelt it out for readability in the sphinx docs but went against my own reasoning in keeping ld.py the same....so again no strong feelings here. I guess most people using this package will know what rf stands for.

gnomad/utils/annotations.py

gnomad/sample_qc/ancestry.py

…utput to expression in/output

mike-w-wilson · 2020-03-31T13:38:07Z

Regarding @nawatts 's comment on __init__.py, do we want to adopt __all__? If so, are there any submodules we would want to leave out of the imports? I can also make the change suggested and just import the submodules themselves from gnomad.sample_qc import ancestry I do like that this approach clears the namespace up. Any thoughts there @lfrancioli ?

lfrancioli · 2020-03-31T15:42:57Z

I like importing the submodules too

… functionality hadn't found a broader use and only adds 2 lines of code, best to inline it.

jkgoodrich

This looks good, thank you Mike so much for doing this. I pretty much just confirmed that the functions were moved where expected, imports updated (thanks for redoing the import organization!) and looked at Laurent’s changes.

Laurent, thanks for moving/removing the field_utils I feel like we weren’t sure about them and this makes more sense.
For ancestry.py vs. populations.py. I still lean towards ancestry for some reason, but I am not sure why since we refer to everything in there with pops or populations so that might be better.
I do currently use random_forest.pretty_print_runs in UKBB actually.
I personally like random_forest.py better, but fine with either.

jkgoodrich · 2020-04-01T04:04:40Z

gnomad/utils/annotations.py

+    )
+
+
+def bi_allelic_expr(t: Union[hl.Table, hl.MatrixTable]) -> hl.expr.BooleanExpression:


Ah yes, this makes sense to move here from field utils, thanks for moving

jkgoodrich · 2020-04-01T04:46:37Z

gnomad/utils/annotations.py

+logger = logging.getLogger(__name__)
+logger.setLevel(logging.INFO)
+
+ANNOTATIONS_HISTS = {


This feels a little odd here since it ends up being more specific to the given QC pipeline (granted not for all) @lfrancioli what do you think?.

I think I'm fine with having this here as in my experience these values are good starting points at least and the different metrics are pretty standard. Totally open to suggestions though -- did you have another module in mind?
I think adding a comment about this is would be nice (but I can see this being outside the scope of this PR and part of another effort to document constants)

jkgoodrich · 2020-04-01T05:13:00Z

gnomad/utils/file_utils.py

                        v.dtype in {hl.tstr, hl.tint32, hl.tfloat32, hl.tint64, hl.tfloat64, hl.tbool}})


-def ht_to_vcf_mt(


Yeah I guess it does make since to move this to a utils.vcf because it is specific to writing to a vcf and since we are planning to add more to the vcf module at somepoint. Good catch

jkgoodrich · 2020-04-01T05:31:05Z

gnomad/utils/vcf.py

+
+    # Create an MT with no cols so that we acn export to VCF
+    info_mt = info_ht.to_matrix_table_row_major(columns=['s'], entry_field_name='s')
+    return info_mt.filter_cols(False)


looks like github is complaining about no newline at end of file

lfrancioli · 2020-04-01T15:28:30Z

Thanks for weighing in @jkgoodrich ! A couple of things based on your comments:
1 . Let's keep ancestry.py :)
2. I'm in for random_forest.py
3. Regarding random_forest.pretty_print_runs, let's keep it there then if you're using it

So I guess what I'm saying is that the current state of things is good with me ;)

mike-w-wilson · 2020-04-02T13:00:12Z

@laurent, one last thing before I run Black and merge! I've moved pop_colors to ancestry. I left all pops and subpops in the dictionary and kept the lower case pop names. I thought someone had mentioned switching to uppercase, which would match the pop_names dictionary, but I could not find it in slack..I'm fine either way but do think matching the pop_names keys would make sense, though I know this goes against history.

lfrancioli · 2020-04-02T13:09:21Z

This is awesome @mike-w-wilson

So I think we should go all lowercase. Also, could you update the names with the more complete ones (and double check the colors match) those here:
https://github.com/macarthur-lab/gnomad_lof/blob/master/R/constants.R

Note that the Amish isn't in the R code as it's new for v3, so needs to be added. If you see anything else that's interesting to have w.r.t. pops in these constants please transfer over too!

mike-w-wilson · 2020-04-02T19:37:42Z

@laurent, added the complete pop names and pop colors but removed any unused pops with clashes.

lfrancioli

Well done!

mike-w-wilson added 4 commits March 25, 2020 17:55

sample_qc top level reorg

2faac54

variant_qc top level reorg

1cf00a2

utils top level reorg

088afaf

removed empty and old utils

bf0c39b

mike-w-wilson requested review from gtiao, jkgoodrich, lfrancioli and nawatts March 25, 2020 22:22

Merge branch 'master' into mw/repo_reorg

3f4556d

nawatts approved these changes Mar 26, 2020

View reviewed changes

mike-w-wilson and others added 4 commits March 26, 2020 09:37

Missed this import in resources

417f922

changed from warnings to logging

987fba7

Updated changelog

d5bf7b9

A few changes to the new repo organization:

f2f9ec2

- removed field_utils.py (functions were moved elsewhere) - moved vcf-related functionalities to vcf.py (I envision this will become the place for all VCF needs)

lfrancioli reviewed Mar 30, 2020

View reviewed changes

mike-w-wilson commented Mar 30, 2020

View reviewed changes

gnomad/utils/annotations.py Outdated Show resolved Hide resolved

gnomad/sample_qc/ancestry.py Outdated Show resolved Hide resolved

This was referenced Mar 30, 2020

Add getting started documentation #210

Merged

Add documentation for using VEP utilities #212

Merged

Updated doc of unphase_call_expr to reflect its change from MT in/o…

7a98c26

…utput to expression in/output

lfrancioli and others added 3 commits March 31, 2020 11:57

unfolded expand_pd_array_col in assign_population_pcs. Since this…

4d1a31e

… functionality hadn't found a broader use and only adds 2 lines of code, best to inline it.

Replaced flip_base function with hail built-in

1425482

Update __init__ to import submodules

41eba06

jkgoodrich reviewed Apr 1, 2020

View reviewed changes

Moved pop_colors to ancestry module

de4e807

Added pop_colors and complete pop name descriptions

297e555

lfrancioli approved these changes Apr 2, 2020

View reviewed changes

Repo formatted using black

9cb2fa2

mike-w-wilson force-pushed the mw/repo_reorg branch from faa5345 to 9cb2fa2 Compare April 3, 2020 13:39

Moved pylint comment back to original line

ab95451

mike-w-wilson merged commit bf9d93d into master Apr 3, 2020

mike-w-wilson deleted the mw/repo_reorg branch April 3, 2020 14:19

This was referenced Apr 3, 2020

Split up utils.generic module #180

Closed

Document constants in constants.py #179

Closed

nawatts mentioned this pull request Aug 18, 2021

Remove module imports from __init__.py files #404

Merged

		)


		def bi_allelic_expr(t: Union[hl.Table, hl.MatrixTable]) -> hl.expr.BooleanExpression:

		v.dtype in {hl.tstr, hl.tint32, hl.tfloat32, hl.tint64, hl.tfloat64, hl.tbool}})


		def ht_to_vcf_mt(

Repo reorganization #207

Repo reorganization #207

Uh oh!

Conversation

mike-w-wilson commented Mar 25, 2020

Summary

Uh oh!

nawatts left a comment

Choose a reason for hiding this comment

Uh oh!

lfrancioli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-w-wilson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mike-w-wilson commented Mar 31, 2020

Uh oh!

lfrancioli commented Mar 31, 2020

Uh oh!

jkgoodrich left a comment

Choose a reason for hiding this comment

Uh oh!

jkgoodrich Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

jkgoodrich Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

lfrancioli Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

jkgoodrich Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

jkgoodrich Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

lfrancioli commented Apr 1, 2020

Uh oh!

mike-w-wilson commented Apr 2, 2020

Uh oh!

lfrancioli commented Apr 2, 2020

Uh oh!

mike-w-wilson commented Apr 2, 2020

Uh oh!

lfrancioli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lfrancioli left a comment •

edited

Loading