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

Expand CLI #208

Merged
merged 15 commits into from Feb 5, 2020
Merged

Expand CLI #208

merged 15 commits into from Feb 5, 2020

Conversation

adrosenbaum
Copy link
Contributor

@adrosenbaum adrosenbaum commented Dec 10, 2019

We need to decouple chanjo from cg. This requires that all operations that cg does through the chanjo API can be made from the CLI.

** How to test **

  1. Enter hasta
  2. Install this branch in its separate environment
  3. For a group/sample that exists in the database do
    • chanjo db samples -g <case_id> --pretty
    • chanjo db samples -s <sample_id> --pretty
    • chanjo calculate coverage -s <sample_id_1> -s <sample_id_2> --omim --pretty

Expected results

chanjo db samples -g <case_id> --pretty should return all samples in the case
chanjo db samples -s <sample_id> --pretty should return the sample
chanjo calculate coverage -s <sample_id_1> -s <sample_id_2> --omim --pretty Should return the mean coverage, and completeness for the samples

Review:

This is a minor version change because new functionality is added in a backward compatible manner

Copy link
Contributor

@moonso moonso left a comment

Choose a reason for hiding this comment

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

Nice job @adrosenbaum . Could you add some tests for the new mixin classes? Also there is a dependency problem on travis, maybe you saw it?

TranscriptStat.transcript,
).filter(
TranscriptStat.sample_id.in_(sample_ids),
Transcript.gene_id.in_(OMIM_GENE_IDS),
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked the performance for this query? Feels like it can take some time to calculate?

chanjo/cli/db.py Outdated


@db_cmd.command()
@click.option('--group-id', '-g')
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a help what this option means?

@click.option('--sample-id', '-s')
@click.option('--pretty', '-p', is_flag=True)
@click.pass_context
def samples(context, group_id, sample_id, pretty):
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if samples=None or of the sample does not exists? Could you add a test or two?

@moonso
Copy link
Contributor

moonso commented Jan 22, 2020

You feel ready with this @adrosenbaum ?

@adrosenbaum
Copy link
Contributor Author

You feel ready with this @adrosenbaum ?

Yes, I'm only working on some listing issues I missed. However I'm thinking of testing this before by installing chanjo on its own condo environment on hasta. I'll let you know when this is done.

@coveralls
Copy link

coveralls commented Jan 22, 2020

Coverage Status

Coverage decreased (-0.1%) to 99.522% when pulling c46b1ea on expand-cli into 397814b on master.

@adrosenbaum
Copy link
Contributor Author

@moonso this is ready to be deployed I think.

Copy link
Contributor

@moonso moonso left a comment

Choose a reason for hiding this comment

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

Looks good!

@adrosenbaum adrosenbaum merged commit 0e705c8 into master Feb 5, 2020
@adrosenbaum adrosenbaum deleted the expand-cli branch February 10, 2020 07:42
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