search

ga4gh / va-spec

An information model for representing variant annotations.
Apache License 2.0
17 stars 4 forks source link

Resolve inheritance discrepancies between core-im-source yaml and the original/informal core-im spec. #135

Open mbrush opened 5 months ago

mbrush commented 5 months ago

Different inheritance of some classes in the initial core-im-source yaml,compared to inheritance in the original/informal core-im specification. e,g, in the current core-im yaml:

If this was done because inheritance from core im classes with their full set of properties was deemed unwanted/problematic in the context of profiles that use these classes to instantiated data – we should consider solutions that are able to maintain the correct/intended hierarchy of classes.

larrybabb commented 4 months ago

I'll defer to @ahwagner on this one. My guess is that Method was an oversight. But I think we intentionally wanted Document to be mappable. Again, this requires you (@mbrush) to discuss with @ahwagner .

larrybabb commented 4 months ago

@mbrush based on our meetings with each other on this topic and my review with @ahwagner . We have adjusted the core-im as follows:

I think there may be more, but the above should have moved us closer to where we want to be.

As far as the gks-common.core-im-source.yaml file, I recognize this could trigger a negative reaction from you as we tried this once before. But I really think we should consider this to be an essential part of the Core Info Model for GKS. Just because these top-level classes are available to all GKS repositories does not change the fact that they are the building blocks upon which the va-spec is able to further build out the core-im for the Statement and StudyResult profiles we are looking to build. I'm sure we will ultimately be moving things around as we all start to see how the GKS repos develop. But I feel that it is important to consider the GKS-common.core-im and va-spec.core-im to be all part of the same thing.

mbrush commented 4 months ago

Thanks for all this work Larry. Most of this i am on board with. A few things I am proposing to change:

  1. InformationEntity was moved to the gks_common.core-im as it was referenced by Contribution.contributionMadeTo

I don't think we need to or want to do this. First, I don't think we should even include the contributionMadeTo property in the model right now. There is no real use case for it, as Contributions are always referenced from the artifacts contributed to, not the other way. But even if we do include this property, I think that in the context of gks commons we want to bind it to Entity rather than InformationEntity - to give it broader utility in describing contributions to a wider range of artifacts.

Given all this, we can keep InformationEntity in the core-im (because it would no longer be referenced by Contribution in gks-common). If we do want the contributionMadeTo property in the core-im, we can always further 'extend' it there to take an InformationEntity as its value. But for now I prefer it in the va-spec core-im.

  1. With InformationEntity back in the va core-im file, there is no need to move Method and Document into gks common (as these will no longer be referenced by any properties in gks-common). We can move these back into va core-im along with InformationEntity, so we can keep the clear dividing line of va-spec starting with InformationEntity and its subtypes.
  1. Finally, since Contribution inherits from Activity, we need to move Activity into gks-commons.

So, I made PR#32, PR#33, and PR#34 in gks-commons, and PR#167 in va-spec to implement the changes needed to achieve all of the above suggestions. @larrybabb we can review next time we talk.