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: do not allow SequenceLocation to have negative start/end v… #443

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

korikuzma
Copy link
Contributor

…alues

close #442

  • Add model_validators to Range and SequenceLocation

@korikuzma korikuzma added bug Something isn't working priority:medium Medium priority labels Aug 28, 2024
@korikuzma korikuzma self-assigned this Aug 28, 2024
@korikuzma korikuzma requested review from a team as code owners August 28, 2024 11:25
@korikuzma korikuzma marked this pull request as draft August 28, 2024 11:29
…alues

close #442

* Add model_validators to `Range` and `SequenceLocation`
@korikuzma korikuzma marked this pull request as ready for review August 28, 2024 11:38
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.

start < end should not be enforced, unless SequenceReference.circular = False. Though it would be nice to raise a warning if end < start and SequenceReference.circular is undefined. A discussion on this topic was first raised in #362, and later moved ga4gh/vrs#477. The VRS documentation doesn't capture this (ope), but it will be necessary to represent variants spanning the origin.

This isn't described in the VRS documentation, opened ga4gh/vrs#537 to address. Just providing a heads-up here as this PR contains some validation code corresponding to this issue.

@@ -454,15 +471,55 @@ class SequenceLocation(_Ga4ghIdentifiableObject):
)
start: Optional[Union[Range, int]] = Field(
None,
description='The start coordinate or range of the SequenceLocation. The minimum value of this coordinate or range is 0. MUST represent a coordinate or range less than the value of `end`.',
description='The start coordinate or range of the SequenceLocation. The minimum value of this coordinate or range is 0. MUST represent a coordinate or range less than or equal to the value of `end`.',
Copy link
Member

Choose a reason for hiding this comment

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

Like that this needs to be >= 0. There should not be a constraint on start < end for VRS 2.0, but just noted that this wasn't updated in the VRS 2.0 field definitions. Will address presently.

)
end: Optional[Union[Range, int]] = Field(
None,
description='The end coordinate or range of the SequenceLocation. The minimum value of this coordinate or range is 0. MUST represent a coordinate or range greater than the value of `start`.',
description='The end coordinate or range of the SequenceLocation. The minimum value of this coordinate or range is 0. MUST represent a coordinate or range greater than or equal to the value of `start`.',
Copy link
Member

Choose a reason for hiding this comment

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

Like that this needs to be >= 0. There should not be a constraint on start < end for VRS 2.0, but just noted that this wasn't updated in the VRS 2.0 field definitions. Will address presently.

@korikuzma
Copy link
Contributor Author

@ahwagner can you re-review?

@korikuzma korikuzma merged commit 918d7a2 into main Sep 17, 2024
6 checks passed
@korikuzma korikuzma deleted the issue-442 branch September 17, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:medium Medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SequenceLocation allows for negative start/end coordinates
2 participants