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

feat: carma outlier detection method #281

Merged
merged 22 commits into from
Jan 4, 2024
Merged

feat: carma outlier detection method #281

merged 22 commits into from
Jan 4, 2024

Conversation

d0choa
Copy link
Collaborator

@d0choa d0choa commented Nov 27, 2023

CARMA outlier detection method:

  • Migration of the method from @addramir repo
  • Documentation page
  • Testing

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (42b366c) 85.67% compared to head (20d4510) 86.84%.
Report is 35 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #281      +/-   ##
==========================================
+ Coverage   85.67%   86.84%   +1.17%     
==========================================
  Files          89       92       +3     
  Lines        2101     2433     +332     
==========================================
+ Hits         1800     2113     +313     
- Misses        301      320      +19     
Files Coverage Δ
src/airflow/dags/common_airflow.py 90.38% <ø> (ø)
src/airflow/dags/finngen_preprocess.py 100.00% <100.00%> (ø)
src/otg/dataset/l2g_feature_matrix.py 82.92% <ø> (+7.31%) ⬆️
src/otg/dataset/study_locus.py 96.20% <100.00%> (+0.04%) ⬆️
src/otg/datasource/eqtl_catalogue/study_index.py 85.00% <ø> (ø)
src/otg/datasource/finngen/study_index.py 100.00% <100.00%> (ø)
src/otg/datasource/finngen/summary_stats.py 100.00% <100.00%> (ø)
src/otg/datasource/gwas_catalog/study_index.py 100.00% <ø> (ø)
src/otg/datasource/ukbiobank/study_index.py 100.00% <ø> (ø)
src/otg/l2g.py 58.06% <ø> (ø)
... and 8 more

d0choa and others added 3 commits November 27, 2023 17:24
Co-authored-by: Yakov Tsepilov <[email protected]>
Co-authored-by: Yakov Tsepilov <[email protected]>
@addramir addramir marked this pull request as ready for review December 21, 2023 00:03
@d0choa
Copy link
Collaborator Author

d0choa commented Dec 21, 2023

You can easily get 100% testing coverage by adding additional examples that evaluate the conditions (if statements) that you are currently missing.

Have a look at the red lines here:
https://app.codecov.io/gh/opentargets/genetics_etl_python/pull/281/blob/src/otg/method/carma.py

@addramir
Copy link
Contributor

@d0choa I can add two more tests to cover red lines 288-296. But not for others - specific extreme cases that very hard to reach since they are using intermediate output of the functions.

@addramir
Copy link
Contributor

addramir commented Dec 21, 2023

@d0choa I have added two tests. For now this is the biggest coverage I can get.

@d0choa d0choa requested a review from DSuveges December 21, 2023 16:12
@d0choa
Copy link
Collaborator Author

d0choa commented Dec 21, 2023

@DSuveges we need approval here

Copy link
Contributor

@tskir tskir left a comment

Choose a reason for hiding this comment

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

I added some minor style comments, but overall everything looks good to me

src/otg/method/carma.py Outdated Show resolved Hide resolved
src/otg/method/carma.py Outdated Show resolved Hide resolved
src/otg/method/carma.py Outdated Show resolved Hide resolved
src/otg/method/carma.py Outdated Show resolved Hide resolved
@addramir
Copy link
Contributor

addramir commented Jan 4, 2024

@DSuveges could you please help to resolve the conflict in poetry.lock?

addramir and others added 3 commits January 4, 2024 16:15
@addramir addramir merged commit 2121982 into dev Jan 4, 2024
3 checks passed
@addramir addramir deleted the ytdo_carma_method branch January 4, 2024 16:21
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.

5 participants