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

Remove KeyType #20

Closed
augustearth opened this issue Sep 23, 2020 · 3 comments
Closed

Remove KeyType #20

augustearth opened this issue Sep 23, 2020 · 3 comments
Assignees

Comments

@augustearth
Copy link
Collaborator

augustearth commented Sep 23, 2020

KeyType was important for Carnival v0 when there were complex data operations that consumed and produced data tables. With the introduction of the graph in Carnival v1, these operations are done in the graph. KeyType is no longer central and should be removed for simplicity's sake.

There is overlap here with secondaryIdFieldMap of GenericDataTable and MappedDataTable. Like KeyType, the secondaryIdFieldMap was important for data table operations. Because secondaryIdFieldMap depends on KeyType, it is not possible to remove KeyType without removing secondaryIdFieldMap.

@augustearth augustearth self-assigned this Sep 23, 2020
@augustearth
Copy link
Collaborator Author

augustearth commented Sep 23, 2020

Affected Files

project filename
clinical EncounterGroup
clinical PatientGroup
clinical OmopVine
clinical EncounterGroupSpec
clinical FrequencyMatcher
clinical FrequencyMatcherSpec
core DataSetDescriptorGraphSpec
core CachingVineSpec
core VineSpec
util KeyType
util GenericDataTable
util MappedDataTable
util FeatureReport
util TabularReport
util GenericDataTableSpec
util MappedDataTableSpec
util FeatureReportSpec
util TabularReportSpec

@augustearth
Copy link
Collaborator Author

augustearth commented Sep 23, 2020

  • Move FrequencyMatcher from Core to Clinical.

@augustearth
Copy link
Collaborator Author

It turns out that EncounterGroup and PatientGroup rely on KeyType for some core functionality. So, rather than remove KeyType completely, I moved it up to carnival.clinical.util and left it in use by PatientGroup and EncounterGroup. I had already removed it from FrequencyMatcher.... which I suspect is a class that will be rethought anyways.

augustearth added a commit that referenced this issue Sep 25, 2020
augustearth added a commit that referenced this issue Sep 25, 2020
augustearth added a commit that referenced this issue Sep 25, 2020
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

No branches or pull requests

1 participant