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!: update to vrs 2.0 models #166

Merged
merged 89 commits into from
Aug 2, 2024
Merged

feat!: update to vrs 2.0 models #166

merged 89 commits into from
Aug 2, 2024

Conversation

katiestahl
Copy link
Contributor

@katiestahl katiestahl commented Jul 17, 2024

close #95

src/fusor/fusor.py Outdated Show resolved Hide resolved
src/fusor/fusor.py Outdated Show resolved Hide resolved
src/fusor/models.py Outdated Show resolved Hide resolved
id: CURIE | None = Field(None, alias="_id")
label: StrictStr | None = None
sequence_location: LocationDescriptor | None = None

_get_id_val = field_validator("id")(return_value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wanted to draw attention to these - it looks like everywhere return_value was used was verifying a CURIE. This is no longer needed. (And note to myself: I will make sure this is tested and works properly with the usage of CURIE from gene normalizer schema)

@korikuzma korikuzma self-requested a review July 18, 2024 18:58
@korikuzma
Copy link
Member

I'll review this tomorrow morning

@katiestahl katiestahl removed the stale label Jul 29, 2024
@katiestahl katiestahl requested a review from korikuzma July 29, 2024 14:17
Copy link
Member

@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.

Nice job 🚀

Copy link
Member

@jsstevenson jsstevenson left a comment

Choose a reason for hiding this comment

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

👍 looks good

Copy link
Member

@ahwagner ahwagner left a comment

Choose a reason for hiding this comment

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

I'm still seeing a 1nt SequenceLocation in the examples here–needs to be addressed.

"type": "SequenceReference"
},
"start": 23253980,
"end": 23253981
Copy link
Member

Choose a reason for hiding this comment

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

I agree–did this get addressed?

@jsstevenson
Copy link
Member

@ahwagner #171

@ahwagner
Copy link
Member

@ahwagner #171

Okay, is that issue out-of-scope for this PR, then?

@ahwagner
Copy link
Member

ahwagner commented Jul 29, 2024

Just a note that, even though (I assume) we’re splitting out the changes to the genomic sequence start/end fields to use VRS 2.0 sequence locations in #171, that the genomic locations of n:n+1 in the examples of #166 are incorrect, even under the original model; an interresidue coordinate that does not include a nt (as needed to describe junction locations) has a location of n:n. These examples will need to be addressed accordingly.

@jsstevenson
Copy link
Member

an interresidue coordinate that does not include a nt (as needed to describe junction locations) has a location of n:n. These examples will need to be addressed accordingly.

yeah, I think we're conceiving of this as an existing error that is orthogonal to the aim of this PR, and a follow-up that overhauls how we're setting location starts/ends will address it exhaustively in #171

@katiestahl katiestahl requested a review from ahwagner July 30, 2024 13:48
@katiestahl katiestahl dismissed ahwagner’s stale review July 30, 2024 13:49

addressing SequenceLocation start/end in issue #171

Copy link
Contributor

@jarbesfeld jarbesfeld left a comment

Choose a reason for hiding this comment

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

Approving the changes with the understanding that we will update for the correct use of start and end in a later PR

…n (same casing as the fusion classes)"

This reverts commit 2880e18.
Copy link

github-actions bot commented Aug 2, 2024

This PR is stale because it has been open 1 day(s) with no activity. Please review this PR.

@github-actions github-actions bot added the stale label Aug 2, 2024
@korikuzma korikuzma removed the stale label Aug 2, 2024
@jsstevenson jsstevenson merged commit 240bbff into main Aug 2, 2024
4 checks passed
@jsstevenson jsstevenson deleted the issue-95-take2 branch August 2, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use latest VRS version
5 participants