-
Notifications
You must be signed in to change notification settings - Fork 36
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
Seqref updates #573
Seqref updates #573
Conversation
$ref: "#/$defs/SequenceString" | ||
description: >- | ||
A :ref:`SequenceString` that is a literal representation of the referenced sequence. | ||
moleculeType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmprocknow concern about "molecule type"; perhaps "molecularContext" would be cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can sort this out during public review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RefSeq calls this Molecule Type, so seems appropriate to keep.
moleculeType: | ||
type: string | ||
enum: | ||
- genomic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general discussion on genomic expressed concerns, possible alternate is "template"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except "template" is a total non-starter. There are genomic variants that are not necessarily going to be involved as template sequence (e.g. variants in regulatory elements)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Group consensus: we should cover the same concept types that RefSeq uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
schema/vrs/def/SequenceReference.rst
Outdated
- | ||
- {'$ref': '#/$defs/sequenceString'} | ||
- 0..1 | ||
- A :ref:`SequenceString` that is a literal representation of the referenced sequence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be lowercase sequenceString
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
- A :ref:`SequenceString` that is a literal representation of the referenced sequence. | ||
* - moleculeType | ||
- | ||
- string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MappableConcept? or are you good with an enum here? I defer to your judgement on this. I almost think we should avoid introducing MappableConcept into VRS unless it really needs to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forget this. enum works for this one.
$ref: "#/$defs/SequenceString" | ||
description: >- | ||
A :ref:`SequenceString` that is a literal representation of the referenced sequence. | ||
moleculeType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can sort this out during public review.
moleculeType: | ||
type: string | ||
enum: | ||
- genomic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Addresses #567 and #568. For discussion.