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

design refinement of existing CatVrs recipes #64

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

larrybabb
Copy link
Contributor

  • embed *Properties into recipes
  • refine DefiningContext to DefiningAllele & DefiningLocation
  • redraft CategoricalCnv on recent design changes

* refine DefiningContext to DefiningAllele & DefiningLocation
* redraft CategoricalCnv on recent design changes
@@ -42,6 +42,14 @@ $defs:
ordered: false
items:
$ref: "#/$defs/Constraint"
mappings:
Copy link
Collaborator

@DanielPuthawala DanielPuthawala Nov 5, 2024

Choose a reason for hiding this comment

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

@larrybabb I'm think we should reevaluate whether it makes sense to keep mappings in CategoricalVariant.

Copy link
Collaborator

@DanielPuthawala DanielPuthawala left a comment

Choose a reason for hiding this comment

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

Documentation re: the new relations needs to me inserted. I have some questions about the division of labor between the new constraints, and what we're doing with the GeneContextConstraint. Did that get axed?

Comment on lines +63 to 66
- $ref: "#/$defs/DefiningAlleleContext"
- $ref: "#/$defs/DefiningLocationContext"
- $ref: "#/$defs/CopyCountConstraint"
- $ref: "#/$defs/CopyChangeConstraint"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that the word constraint was dropped from the DefiningAlleleContext and DefiningLocationcontext, but should we be consistent and make the drop the word constraint from the others as well or is there a rationale for distinguishing between them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, have we discussed yet updating the CopyCountConstraint and CopyChangeConstraint into a single copy constraint along the lines of the most recent iteration of that constraint in the discussion here?

Comment on lines +104 to +107
``sequence_liftover`` refers to variants or locations that represent a congruent concept on a differing assembly of a
human genome (e.g. "GRCh37" and "GRCh38") or gene (e.g. Locus Reference Genomic) sequence. ``transcript_projection``
refers to variants or locations that occur on transcripts projected from the defined genomic concept. ``codon_translation``
refers to variants or locations that translate from the codon(s) represented by the defined concept.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are now out of date. Need to update with descriptions appropriate to the nee;y enumerated relations. @ahwagner what is translates_through?

inherits: Constraint
description: >-
The defining allele and its associated relationships that are congruent
with member variants.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given recent thinking wrt hypothetical lower-order AdjacencyConstraint, which takes, eg, two vrs:Location arguments and the stipulates that they are now adjacent (potentially mediated by a linker sequence, etc), the gates are open to the idea that a single vrs object may satisfy be reused to satisfy the arguments of multiple states (which also means the logic is now non-linear, which I don't think is a problem, but I'll make a note in my brain to ponder later).

So, is there a reason to have the dichotomy of a DefiningLocationContext and DefiningAlleleContext instead of a DefiningLocationContext and a DefiningStateContext where the latter state context functions more akin to a sequence or functional entity (like a gene) completely divorced from a location in the genome, and therefore a system where a vrs:Allele would be sufficient to satisfy both the DefiningLocationContext and DefiningStateContext, but a vrs:Location would only satisfy the DefiningLocationContext?

Or put another way, in splitting out the DefiningContextConstraint, we now have one constraint that only handles location information, and another that... does the smae thing as the prior DefiningContextConstraint.

description: >-
Defined relationships between members of the categorical variant and the defining context.
Defined relationships from which members relate to the defining context.
``sequence_liftover`` refers to variants or locations that represent a congruent concept on a differing assembly of a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto above, this needs to be updated in line with the new relations.

``sequence_liftover`` refers to variants or locations that represent a congruent concept on a differing assembly of a
human genome (e.g. "GRCh37" and "GRCh38") or gene (e.g. Locus Reference Genomic) sequence. ``transcript_projection``
refers to variants or locations that occur on transcripts projected from the defined genomic concept. ``codon_translation``
refers to variants or locations that translate from the codon(s) represented by the defined concept.
matchCharacteristic:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am excited to see what this gets us.

@DanielPuthawala DanielPuthawala merged commit e9c7ce6 into 1.x Nov 5, 2024
4 checks passed
@DanielPuthawala DanielPuthawala deleted the unconference-update branch November 5, 2024 18:12
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