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(test): update copyChange to use MappableConcept #620

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

korikuzma
Copy link
Contributor

@larrybabb @ahwagner can you also confirm that ga4gh_serialize/ga4gh_digest/ga4gh_identify will remain the same?

@korikuzma korikuzma self-assigned this Dec 18, 2024
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.

@ahwagner I'm fine with this for now because I think that CopyNumberChange will ultimately be dropped from VRS and only included in Cat-VRS. As long as this stays as draft maturity you can do whatever you want with it. This. class will NOT be used by ClinGen as-is. I do think we need to have this discussion and determine if there are any folks that are interested in using this as-is.

@larrybabb
Copy link
Contributor

Also, having structures like MappableConcept included in digest keys that impact identity is a generally bad idea IMO. MappableConcept does not inherently require any fields and is meant to be a useful construct for sharing entities that exist in the public domain that we do not want to take ownership of modeling ourselves.

@larrybabb
Copy link
Contributor

larrybabb commented Dec 18, 2024

Also, Any attributes involved in VRS digest generation that are not "codes" should be strict enumerations that are clearly specified, they should not be something that can be added to, modified or deleted from by external groups.

@ahwagner we probably should state this as a policy behind digest generation.

@korikuzma
Copy link
Contributor Author

Also, having structures like MappableConcept included in digest keys that impact identity is a generally bad idea IMO. MappableConcept does not inherently require any fields and is meant to be a useful construct for sharing entities that exist in the public domain that we do not want to take ownership of modeling ourselves.

@larrybabb so it requires either label or primaryCode to be present, but ya it feels weird to have this included in digest keys if primaryCode can be null

@ahwagner
Copy link
Member

@larrybabb agree that this should be tightly constrained for digests in VRS objects and we should make primaryCode the ga4gh.inherent attribute for MappableConcept. Also agree we should leave this in draft for the moment.

@korikuzma korikuzma merged commit c15310c into 2.0.0-ballot.2024-11 Dec 18, 2024
3 checks passed
@korikuzma korikuzma deleted the fix-copy-change branch December 18, 2024 15:06
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.

3 participants