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: refactor hgvsTools and support c. translation #366

Merged
merged 25 commits into from
Mar 28, 2024
Merged

Conversation

larrybabb
Copy link
Contributor

@larrybabb larrybabb commented Mar 21, 2024

Address #365

Begin refactoring the Translator logic to aggregate the functions and logic related to hgvs and provide support for c. from and to translation. This should be step in the direction of re-thinking the organization of how the Translators are architected.

@jsstevenson
Copy link
Contributor

jsstevenson commented Mar 22, 2024

It would be nice, albeit out of scope here, if the generic dataproxy interface that's redefined (copied?) in VRS-Python from seqrepo was located in a more central package so that we don't do any damage by making these changes here without mirroring them back to their origin

@jsstevenson jsstevenson changed the title Issue 365 - refactor hgvsTools and support c. translation feat: refactor hgvsTools and support c. translation Mar 22, 2024
@larrybabb larrybabb marked this pull request as ready for review March 27, 2024 12:46
@larrybabb larrybabb requested review from a team as code owners March 27, 2024 12:46
@korikuzma
Copy link
Contributor

@larrybabb can you pull in changes from main branch and resolve conflicts first?

@larrybabb
Copy link
Contributor Author

@korikuzma @jsstevenson @ahwagner could you please try to prioritize reviewing this PR? The primary purpose of it was to get support for the c. nomenclature working. I also reverted the hgvs branch dependency that we introduced and will localize our version of it until that hgvs branch is officially merged and a new release is created. I started to consolidate all the hgvs-related logic in the hgvs_tools class, but deferred making any larger strategic design changes until we can meet as a team and agree on what and when we need to do there.

@korikuzma
Copy link
Contributor

korikuzma commented Mar 27, 2024

@larrybabb I am in the middle of reviewing 😄 James is PTO today, but he did an initial review already

Copy link
Contributor

@korikuzma korikuzma left a comment

Choose a reason for hiding this comment

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

@larrybabb Great work! Overall, I like this change. I made some suggestions

notebooks/variation_normalizer_rest_dp.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/dataproxy.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/dataproxy.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/extras/translator.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/extras/translator.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/utils/hgvs_tools.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/utils/hgvs_tools.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/utils/hgvs_tools.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/utils/hgvs_tools.py Outdated Show resolved Hide resolved
src/ga4gh/vrs/utils/hgvs_tools.py Outdated Show resolved Hide resolved
@jsstevenson
Copy link
Contributor

@larrybabb as Kori said, I'm out for at least today (which will probably entail some frantic catch-up once I'm back) but for the sake of pragmatism, we can assume my approval once Kori's additional notes above are addressed

@larrybabb larrybabb requested a review from korikuzma March 28, 2024 01:08
Copy link
Contributor

@korikuzma korikuzma left a comment

Choose a reason for hiding this comment

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

Two minor things. Good job @larrybabb

'refget_accession' (str): The accession ID of the reference genome.
'start' (int): The start position of the allele.
'end' (int): The end position of the allele.
literal_sequence' (str): The literal sequence for the allele.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
literal_sequence' (str): The literal sequence for the allele.
'literal_sequence' (str): The literal sequence for the allele.

sstate = models.LiteralSequenceExpression(sequence=ins_seq)
allele = models.Allele(location=location, state=sstate)
allele = self._post_process_imported_allele(allele, **kwargs)
# TODO: ask other devs if this should be down on all _from_... methods?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# TODO: ask other devs if this should be down on all _from_... methods?

@larrybabb larrybabb merged commit facd457 into main Mar 28, 2024
8 checks passed
@larrybabb larrybabb deleted the issue-365 branch March 28, 2024 14:43
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.

4 participants