monarch-initiative / genophenocorr

Genotype Phenotype Correlation
https://monarch-initiative.github.io/genophenocorr/stable
MIT License
4 stars 1 forks source link

Notebooks all work. Small changes so tables look good. #173

Closed lnrekerle closed 2 weeks ago

lnrekerle commented 3 weeks ago

This PR adds a variant name column that shows the HGVS name of a variant in addition to the chromosomal representation. Currently the PR only works for small variants (HGVS). We will additionally need a function for structural variants. The tutorial.rst was also updated for the change in the variant_effect_count_by_tx function. And finally, the function get_hgvs_cdna_by_tx was added to the Class Variant.

ielis commented 3 weeks ago

Hi @lnrekerle pls request review when this is ready to be looked at and when the required checks pass. Thanks!

pnrobinson commented 3 weeks ago

@lnrekerle Please check why the CI test is failing. It seems there is one item in the documentation that is not up to date

tests/analysis/test_mtc_filter.py: 740 warnings
  /home/runner/work/genophenocorr/genophenocorr/src/genophenocorr/analysis/predicate/phenotype/_pheno.py:153: DeprecationWarning: `is_observed` property was deprecated and will be removed in `v0.3.0`. Use `is_present` instead
    if phenotype.is_observed:

In general we want all tests to pass before we merge a PR, let's try to clean everything up!

pnrobinson commented 3 weeks ago

@lnrekerle Please also write a brief summary of what the PR does. From our discussion yesterday, I think it is the addition of the "Variant name" column. This is nice, and I looked at some of the notebooks and it seems to work. So for instance, please write something like this

"This PR adds a variant name column that shows the HGVS name of a variant in addition to the chromosomal representation." Probably also: "Currently the PR only works for small variants (HGVS). We will additionally need a function for structural variants" (this lets the reviewer know exactly what the PR does!)

lnrekerle commented 2 weeks ago

@ielis Let me know if this fixed the issues! Or just feel free to merge it if it's good.

ielis commented 2 weeks ago

@lnrekerle the merge honors are all yours!