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.

Sign up for GitHub

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

Jump to bottom

feat: drop v2g and reimplement distance features #771

Merged
merged 61 commits into from
Oct 1, 2024
Merged

feat: drop v2g and reimplement distance features #771

merged 61 commits into from
Oct 1, 2024

Conversation

ireneisdoomed
Copy link
Contributor

@ireneisdoomed ireneisdoomed commented Sep 18, 2024 •
edited
Loading

✨ Context

This PR closes 3434 and 3258

The definition of the features have been implemented as agreed with @addramir and @xyg123*. The spreadsheet that documents them has been updated accordingly https://docs.google.com/spreadsheets/d/1wUs1AprRCCGItZmgDhc1fF5BtwCSosdzFv4NQ8V6Dtg/edit?gid=452826388#gid=452826388

🛠 What does this PR implement

  • Removal of V2G as a step and a dataset.
  • We still have the intervals dataset, but we are not dumping it (will be processed by L2G)
  • Generation of distance features based on the variant index, not V2G
  • Generation of negative gold standard based on the variant index, not V2G
  • Addition of distance to footprint features
  • Addition of all features in the neighbourhood
  • Testing

🙈 Missing

  • Discuss the filtering on protein coding genes when calculating the neighbourhood features. To be implemented in another PR @addramir

🚦 Before submitting

  • Do these changes cover one single feature (one change at a time)?
  • Did you read the contributor guideline?
  • Did you make sure to update the documentation with your changes?
  • Did you make sure there is no commented out code in this PR?
  • Did you follow conventional commits standards in PR title and commit messages?
  • Did you make sure the branch is up-to-date with the dev branch?
  • Did you write any new necessary tests?
  • Did you make sure the changes pass local tests (make test)?
  • Did you make sure the changes pass pre-commit rules (e.g poetry run pre-commit run --all-files)?

ireneisdoomed and others added 30 commits September 3, 2024 12:28
@ireneisdoomed
refactor(L2GFeatureMatrix): remove schema validation
50d98ed
@ireneisdoomed
Merge branch 'dev' of https://github.com/opentargets/gentropy into il…
66a0f0b 
…-3252
@ireneisdoomed
refactor(FeatureFactory): reshape feature generation WIP
e1f7c5c
@pre-commit-ci
chore: pre-commit auto fixes [...]
a7757ac
@ireneisdoomed
Merge branch 'dev' of https://github.com/opentargets/gentropy into il…
646a810 
…-3252
@ireneisdoomed
chore: set l2gfeature properties with decorator
8a70bf2
@ireneisdoomed
chore(l2gfeature): make credible_set and input_dependency instance at…
c690ffc 
…tributes
@ireneisdoomed
chore(l2gfeature): make credible_set and input_dependency instance at…
a54e694 
…tributes
@ireneisdoomed
chore(featurefactory): distanceTssMeanFeature working
85a7bf4
@ireneisdoomed
refactor(l2g): improve step dependency management
d24de6d
@ireneisdoomed
feat: implement
6a3af69
@ireneisdoomed
chore: fix mypy issues
09d5291
@ireneisdoomed
Merge branch 'dev' of https://github.com/opentargets/gentropy into il…
6211d8d 
…-3252
@ireneisdoomed
Merge branch 'il-3252' of https://github.com/opentargets/gentropy int…
5561b74 
…o il-3252
@ireneisdoomed
feat: l2gfeaturematrix.from_features_list working
b1f607b
@ireneisdoomed
Merge branch 'dev' of https://github.com/opentargets/gentropy into il…
021e159 
…-3252
@ireneisdoomed
chore: comment out obsolete refs
da20073
@ireneisdoomed
chore(L2GFeatureMatrix): change mode attribute to with_gold_standard
d06c059
@ireneisdoomed
refactor(l2g): move feature matrix writing to training module
0a007a7
@ireneisdoomed
feat(L2GFeatureMatrix): accept L2GGoldStandard or StudyLocus as inputs
abfdf22
@ireneisdoomed
feat: implement methods to build a feature matrix based on a studyloc…
1eed6f3 
…us/L2GGoldStandard instance
@ireneisdoomed
feat: coloc logic prototype
b4a86a1
@ireneisdoomed
feat(l2g): filter non gwas credible sets at the start of the step
0b09193
@ireneisdoomed
feat: rewrite colocalisation feature factory
a60095b
@ireneisdoomed
test: add test_colocalisation_feature_type
16085ad
@ireneisdoomed
test(colocalisation): add test_extract_maximum_coloc_probability_per_…
7ab1ff1 
…region_and_gene
@ireneisdoomed
feat(L2GFeatureInputLoader): support multiple deps by passing loader …
e56e8ea 
…as kwarg
@ireneisdoomed
test: add integration tests test_build_feature_matrix
b8525ad
@ireneisdoomed
Merge branch 'dev' of https://github.com/opentargets/gentropy into il…
ad8481e 
…-3252
@ireneisdoomed
feat(variant_index): add get_distance_to_gene and deprecate `get_di…
e032bae 
…stance_from_tss`
@ireneisdoomed ireneisdoomed marked this pull request as ready for review September 18, 2024 17:04
@d0choa
Copy link
Collaborator

d0choa commented Sep 19, 2024

The new features are definitely a step forward to make it more modular and easy to interpret.

xyg123
xyg123 reviewed Sep 19, 2024
View reviewed changes
src/gentropy/dataset/l2g_feature.py Outdated
.groupBy("studyLocusId", "geneId")
.agg(agg_expr.alias(feature_name))
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using weighted distances as the feature now? instead of the normalised distance score between 0-1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We used to weight them earlier as well. The feature itself is not normalised until later.

xyg123
xyg123 reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@xyg123 xyg123 left a comment

Choose a reason for hiding this comment

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

Looks really nice, good bye v2g, just need to make sure we are happy with the weighted distance features. x

Base automatically changed from il-3252 to dev September 23, 2024 12:30
xyg123
xyg123 reviewed Sep 24, 2024
View reviewed changes
src/gentropy/dataset/intervals.py
@@ -74,32 +71,3 @@ def from_source(
source_class = source_to_class[source_name]
data = source_class.read(spark, source_path) # type: ignore
return source_class.parse(data, gene_index, lift) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, what is the plan with the interval dataclass? Do we plan to have a separate DAG to process these and save them somewhere?

xyg123
xyg123 approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@xyg123 xyg123 left a comment

Choose a reason for hiding this comment

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

Everything looks good!

@ireneisdoomed ireneisdoomed merged commit b7ccfae into dev Oct 1, 2024
5 checks passed
@ireneisdoomed ireneisdoomed deleted the il-3434 branch October 1, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xyg123 xyg123 xyg123 approved these changes

@project-defiant project-defiant Awaiting requested review from project-defiant

Assignees
No one assigned
Labels
Dataset Datasource documentation Improvements or additions to documentation Feature Method size-XL Step
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop V2G step
3 participants
@ireneisdoomed @d0choa @xyg123