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

fix(l2g): direct model path to hf repo when none and align training data filename #997

Merged
merged 5 commits into from
Feb 20, 2025

Conversation

ireneisdoomed
Copy link
Contributor

@ireneisdoomed ireneisdoomed commented Feb 20, 2025

✨ Context

This PR fixes 2 minor bugs that was causing the L2G prediction step to crash when model_path=None:

  • L2GPrediction.from_credible_set defaults to the opentargets/locus_to_gene repo if model_path is none. Now, at the L2G step level this variable matches hf_repo_id as long as download_from_hub is set to true
  • Small typo in the filename of the training set when exporting to HF

🛠 What does this PR implement

🙈 Missing

🚦 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 uv run pre-commit run --all-files)?

@ireneisdoomed ireneisdoomed requested a review from xyg123 February 20, 2025 09:42
@github-actions github-actions bot added bug Something isn't working Method Step size-XS labels Feb 20, 2025
@project-defiant
Copy link
Contributor

@ireneisdoomed do we have to make new release candidate to run the l2g steps ?

@javfg

@ireneisdoomed
Copy link
Contributor Author

@ireneisdoomed do we have to make new release candidate to run the l2g steps ?

@javfg

Not necessarily but it would be the easiest.
The bug is not there if model_path is provided through the configuration (requires changes in orchestration)

@github-actions github-actions bot added size-S and removed size-XS labels Feb 20, 2025
@ireneisdoomed
Copy link
Contributor Author

Train and predict train are now synced, the bug has been fixed.
In a following PR, I'll implement the training data sampling for a faster shapley calculation.

@project-defiant project-defiant merged commit 198019f into dev Feb 20, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Method size-S Step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants