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.

Sign up for GitHub

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

Jump to bottom

Issue 404 #416

Merged
merged 13 commits into from Apr 1, 2023
Merged

Issue 404 #416

merged 13 commits into from Apr 1, 2023

Conversation

ahwagner
Copy link
Member

@ahwagner ahwagner commented Mar 3, 2023

This is a draft PR for the CopyNumber and CopyNumberAssessment classes (revising from the absolute/relative copynumber classes per #404).

@ahwagner
Copy link
Member Author

ahwagner commented Mar 3, 2023

In draft while awaiting EFO term from EBISPOT/efo#1941 and documentation updates.

andreasprlic
andreasprlic reviewed Mar 6, 2023
View reviewed changes
docs/source/terms_and_model.rst Outdated
}

.. _RelativeCopyNumber:
.. _CopyNumberAssessment:
Copy link

Choose a reason for hiding this comment

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

Not the biggest fan of the term assessment, but I like that we are using similar terms as EFO and I don't have any better term either.

korikuzma
korikuzma reviewed Mar 6, 2023
View reviewed changes
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.

Remove extra space

schema/vrs.json Outdated Show resolved Hide resolved
@viveknuthalapati
Copy link

Would we ever want to represent the copy number count and the copy number assessment together? In which case would one of the pieces of information be part of the variation descriptor?

@ahwagner ahwagner marked this pull request as ready for review March 31, 2023 19:19
larrybabb
larrybabb approved these changes Apr 1, 2023
View reviewed changes
Copy link
Contributor

@larrybabb larrybabb left a comment

Choose a reason for hiding this comment

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

I looked it over pretty well via my cell phone so I may have missed something. It looks pretty thorough and accurate for now.

@ahwagner ahwagner merged commit 0c0c977 into main Apr 1, 2023
3 checks passed
@larrybabb
Copy link
Contributor

Would we ever want to represent the copy number count and the copy number assessment together? In which case would one of the pieces of information be part of the variation descriptor?

IMO - When/if this situation happens and I suspect it will, the more precise representation begin CopyNumberCount would be the best representation of the copy number variation and the assessment or change qualifier would be an element of the descriptor for the CopyNumberCount.

andreasprlic
andreasprlic reviewed Apr 3, 2023
View reviewed changes
Copy link

@andreasprlic andreasprlic left a comment

Choose a reason for hiding this comment

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

thank you, looks great

@larrybabb larrybabb deleted the issue-404 branch January 4, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreasprlic andreasprlic andreasprlic left review comments

@korikuzma korikuzma korikuzma left review comments

@larrybabb larrybabb larrybabb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ahwagner @viveknuthalapati @larrybabb @andreasprlic @korikuzma