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

add ui template for ctl #660

Merged
merged 31 commits into from Feb 9, 2022

Conversation

Alexanderlacuna
Copy link
Contributor

Description

This PR integrate to gn3 ctl and making the required plots

How should this be tested?

Any background context you want to provide?

What are the relevant pivotal tracker stories?

Screenshots (if appropriate)

Questions

wqflask/wqflask/ctl/gn3_ctl_analysis.py Outdated Show resolved Hide resolved
wqflask/wqflask/ctl/gn3_ctl_analysis.py Outdated Show resolved Hide resolved
wqflask/wqflask/ctl/gn3_ctl_analysis.py Outdated Show resolved Hide resolved
wqflask/wqflask/views.py Outdated Show resolved Hide resolved
wqflask/wqflask/views.py Outdated Show resolved Hide resolved
@Alexanderlacuna
Copy link
Contributor Author

@BonfaceKilz thanks for the review, need to work on the naming of my variables in general.Will ping you when I fix the above.

@Alexanderlacuna
Copy link
Contributor Author

plus before merge, will set things up for @robwwilliams @zsloan to test this on penguin and give their feedback

@BonfaceKilz
Copy link
Collaborator

BonfaceKilz commented Jan 27, 2022 via email

@BonfaceKilz
Copy link
Collaborator

BonfaceKilz commented Jan 27, 2022 via email

@Alexanderlacuna Alexanderlacuna marked this pull request as ready for review February 2, 2022 06:05
Copy link
Collaborator

@BonfaceKilz BonfaceKilz left a comment

Choose a reason for hiding this comment

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

LGTM. @Alexanderlacuna should I go ahead and merge this? You can address minor issues in another PR.

Comment on lines +29 to +32
def parse_geno_data(dataset_group_name) -> dict:
"""
Args:
dataset_group_name: string name
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: Args should be more descriptive. And types captures what you've documented as an arg:

def parse_geno_data(dataset_group_name -> str) -> dict:
...

I think I'll have to look for resources to point you to show you how to improve documentation. @arunisaac if you have anything in mind, you could share ;)

Comment on lines +101 to +102
"""function to make an api call
to gn3 and run ctl"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: We already know this is a function.

Suggested change
"""function to make an api call
to gn3 and run ctl"""
"""Make an api call to gn3 and run ctl"""

@arunisaac
Copy link
Contributor

arunisaac commented Feb 2, 2022 via email

@BonfaceKilz
Copy link
Collaborator

BonfaceKilz commented Feb 2, 2022 via email

@Alexanderlacuna
Copy link
Contributor Author

@BonfaceKilz This is can be merged

@BonfaceKilz BonfaceKilz merged commit 78a544e into genenetwork:testing Feb 9, 2022
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.

None yet

3 participants