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

Refactor CohortAlleleFrequency to inherit from StudyResult #141

Open
mbrush opened this issue May 28, 2024 · 4 comments
Open

Refactor CohortAlleleFrequency to inherit from StudyResult #141

mbrush opened this issue May 28, 2024 · 4 comments
Assignees

Comments

@mbrush
Copy link
Contributor

mbrush commented May 28, 2024

The root class of this profile is called CohortAlleleFrequency, and looks to be a StudyResult. But it inherits from the core-im InformationEntity class, rather than the `StudyResult' class. Can we fix this?

If there are concerns about inheritance of some of the properties defined on StudyResult in the core IM (e.g. there are too many ones that are not applicable) we should consider solutions that are able to maintain the correct/intended hierarchy of classes (e.g. deriving a core-im slim part of the profiling process)

Also can we name the root class it accordingly, as CohortAlleleFrequencyStudyResult - so we clearly indicate how it relates to / extends the Core IM. This IMO is an important principle to follow in "Standard" profiles, like this caf profile (but does not have to be followed in implementation profiles if you don’t want).

@mbrush mbrush changed the title caf profile: Why does the caf profile inherit from Information Entity rather than Study Result? Why does the caf profile inherit from Information Entity rather than Study Result? May 29, 2024
@larrybabb
Copy link
Contributor

It should inherit from StudyResult. This is outstanding work that I need to do. I will use this ticket as my assignment to get this work completed.

I do not agree with making the class name longer by appending the name of the parent class to the end. We can discuss this again, but if we do this here then we should go throughout all of GKS products and do it everywhere which will create a considerable amount of work for what is very little value IMO.

@larrybabb larrybabb self-assigned this Jun 13, 2024
@larrybabb larrybabb changed the title Why does the caf profile inherit from Information Entity rather than Study Result? Refactor CohortAlleleFrequency to inherit from StudyResult Jun 13, 2024
@mbrush
Copy link
Contributor Author

mbrush commented Jun 14, 2024

Thanks for fixing the inheritance of the CAF class Larry. There are other alignment issues to address for the CAF profile, as documented in tickets field, and my comments in the file here. But we will get to these soon

As for the value added by using more informative names on specialized class and property names in profiles (e.g. Statement --> VariantPathogenicityStatement, not VariantPathogenicity) - you know that I have a different opinion here which I have detailed this previously and won't get into here.

I will however comment on the concern about the amount of work implementing this convention would require, by pointing out that IMO this would be applied only for a few classes/properties created through the VA profiling process. Specifically, Statement class or SPOQ property specializations created in a profile (or analogous properties if it is a StudyResult properties).

These naming conventions do not need to be followed for most non-SPOQ properties, or applied in other GKS specifications where profiling is not done. So there is really not much retroactive work to be done (by my count, we are just talking about adding an extra word to the names of one class name and four property names in the Var Path profile, and to one class name and three property names in the caf profile). It is more about setting this precedent going forward.

@larrybabb
Copy link
Contributor

Thank you @mbrush. Your file and points above will all be included in the work I am doing to lift over CAF.

@larrybabb
Copy link
Contributor

@mbrush please review the revised CohortAlleleFrequencyStudyResult standard profile. I believe this should cover most if not everything from above. If there are any remaining issues, let's make a separate ticket and close this one as a reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants