Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace all ukb_common with ukbb_common #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mkanai
Copy link
Collaborator

@mkanai mkanai commented Nov 2, 2021

I replaced all ukb_common with ukbb_common in this repo. This is dependent on 1) renaming the repo itself (Nealelab/ukbb_common) and 2) maybe some docker update as detailed below.

@mkanai mkanai requested review from konradjk and wlu04 November 2, 2021 20:05
@@ -17,10 +17,10 @@
description="Common functions for UK Biobank Data",
long_description=long_description,
long_description_content_type="text/markdown",
url="https://github.com/Nealelab/ukb_common",
url="https://github.com/Nealelab/ukbb_common",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -8,7 +8,7 @@


MKL_OFF = 'export MKL_NUM_THREADS=1; export MKL_DYNAMIC=false; export OMP_NUM_THREADS=1; export OMP_DYNAMIC=false; '
SCRIPT_DIR = '/ukb_common/saige'
SCRIPT_DIR = '/ukbb_common/saige'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@konradjk I think this line suggests the docker image already has ukb_common in it. Happy to keep ukb_common for now or please update the docker too

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think since I'm the next person to use the dockers, fine to update and I will update when it merges/next time I use it

@mkanai
Copy link
Collaborator Author

mkanai commented Nov 2, 2021

Relatedly, I'm not sure why ./src/ukbb_common/{resources,utils} and ./{resources,utils} are duplicated?

@konradjk
Copy link
Collaborator

konradjk commented Nov 3, 2021

Relatedly, I'm not sure why ./src/ukbb_common/{resources,utils} and ./{resources,utils} are duplicated?

I believe this was in order to submit it as a package to PyPI. @wlu04 is there a way to consolidate them now that we're renaming the whole thing?

@wlu04
Copy link
Contributor

wlu04 commented Nov 3, 2021

Relatedly, I'm not sure why ./src/ukbb_common/{resources,utils} and ./{resources,utils} are duplicated?

I did this because this enables from ukbb_common import function(). Otherwise I will have to use from ukbb_common.resources import function(). I sure can take a look and see if there's a simpler way.

@konradjk
Copy link
Collaborator

konradjk commented Nov 3, 2021

I think if we put a __init__.py that has from .resources import * etc, that could work

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.

3 participants