Add setup module as module, not packaged

malariagen / ag1000g-phase3-data-paper

Other

1 stars 2 forks source link

Add setup module as module, not packaged #4

Closed hardingnj closed 4 years ago

hardingnj commented 4 years ago

As discussed on call

Decided to keep class structure, but simplify to module.

review-notebook-app[bot] commented 4 years ago

Check out this pull request on

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

leehart commented 4 years ago

I'm hesitating because release in release_data() can be a verb, and I still have much to learn about Python best practices, amongst other things.

hardingnj commented 4 years ago

Cool- happy to rename for clarity. What's the PEP guideline about verbs?

alimanfoo commented 4 years ago

LGTM. Couple of soft suggestions, happy with whatever you prefer:

Still reckon you could do without a class and just provide functions and variables.
Could just name module "ag3" if you wanted to be really concise.
Could shorten function names a little.
Total nit, I know this is something we go back and forth on, but prefer "sample_set" to "sampleset".

E.g.:

import ag3
ag3.all_sample_sets
ag3.wild_sample_sets
ag3.samples(...)
ag3.site_filters(...)
ag3.snp_variants(...)
ag3.snp_calldata(...)

cclarkson commented 4 years ago

Hi guys, any idea on an eta for the review on this - just wondering if it is worth me re-writing the map code to work without the module or wait for it to be merged?

alimanfoo commented 4 years ago

Happy for @hardingnj to merge as-and-when sees fit.

hardingnj commented 4 years ago

I've incorporated Alistair's suggestions, so please feel free to review + merge.