Skip to content

Annotate_Cohort_Upgrade#27

Merged
Johnnyassaf merged 11 commits intomainfrom
Cohort-Annotation-Upgrade
Feb 16, 2026
Merged

Annotate_Cohort_Upgrade#27
Johnnyassaf merged 11 commits intomainfrom
Cohort-Annotation-Upgrade

Conversation

@Johnnyassaf
Copy link
Contributor

Adding a new optional annotation to the seqr loader pipeline

Proposed Changes

  • Changing the Annotate Cohort Function to accept optional additions

Checklist

  • Related GitHub Issue created
  • Tests covering new change
  • Linting checks pass

Copy link
Collaborator

@MattWellie MattWellie left a comment

Choose a reason for hiding this comment

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

this will work, but for cleanliness can you leave the existing code intact and add an extra method to apply this extra annotation?

AFAIK behind the scenes there's no difference between annotating once with two sources, or annotating twice each with a different source. The fields are all resolved in a lazy way, so it wouldn't be any less efficient to add a second (conditional) mt.annotate_rows() call.

Main reason for implementing this without touching what's already there is so that if something breaks, or something related to this is strange in future, we can use git blame to identify which lines changed, and when, to help debug. If all these lines are changed, it's ambigious whether the new content, or a change to the old content, is responsible for any issues, as they would both be affected by the same change. Adding a new merge commit with just the ~10-20 lines implementing the new functionality is much more useful when trying to track down the source of problems.

…tional annotation in annotate_cohort

Co-authored-by: Matt Welland <mattwellie@gmail.com>
@MattWellie MattWellie requested a review from EddieLF February 16, 2026 05:22
Copy link
Collaborator

@MattWellie MattWellie left a comment

Choose a reason for hiding this comment

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

can you add a version bump to this before we merge it in?

remove unnecessary elements

Co-authored-by: Matt Welland <mattwellie@gmail.com>
Copy link
Collaborator

@MattWellie MattWellie left a comment

Choose a reason for hiding this comment

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

AEIOU!

@Johnnyassaf Johnnyassaf merged commit 6687eb4 into main Feb 16, 2026
4 checks passed
@Johnnyassaf Johnnyassaf deleted the Cohort-Annotation-Upgrade branch February 16, 2026 06:05
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.

2 participants