-
Notifications
You must be signed in to change notification settings - Fork 4
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
Comments
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. |
CohortAlleleFrequency
to inherit from StudyResult
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. 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. |
Thank you @mbrush. Your file and points above will all be included in the work I am doing to lift over CAF. |
@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. |
The root class of this profile is called
CohortAlleleFrequency
, and looks to be a StudyResult. But it inherits from the core-imInformationEntity
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).The text was updated successfully, but these errors were encountered: