feat: refactor RetrieveCohortOverlapStats and fix breakdown stats method

...and fix model tests

Author: pieterlukasse

controllers/cohortdata.go

@@ -282,13 +282,12 @@ func populateConceptValue(row []string, cohortItem models.PersonConceptAndValue,
 func (u CohortDataController) RetrieveCohortOverlapStats(c *gin.Context) {
 	errors := make([]error, 4)
 	var sourceId, caseCohortId, controlCohortId int
-	var conceptIdsAndValues []utils.CustomConceptVariableDef
-	var cohortPairs []utils.CustomDichotomousVariableDef
+	var conceptDefsAndCohortPairs []interface{}
 	sourceId, errors[0] = utils.ParseNumericArg(c, "sourceid")
 	caseCohortId, errors[1] = utils.ParseNumericArg(c, "casecohortid")
 	controlCohortId, errors[2] = utils.ParseNumericArg(c, "controlcohortid")
-	conceptIdsAndValues, cohortPairs, errors[3] = utils.ParseConceptDefsAndDichotomousDefs(c)
-	conceptIds := utils.ExtractConceptIdsFromCustomConceptVariablesDef(conceptIdsAndValues)
+	conceptDefsAndCohortPairs, errors[3] = utils.ParseConceptDefsAndDichotomousDefsAsSingleList(c)
+	_, cohortPairs := utils.GetConceptDefsAndValuesAndCohortPairsAsSeparateLists(conceptDefsAndCohortPairs)
 
 	validAccessRequest := u.teamProjectAuthz.TeamProjectValidation(c, []int{caseCohortId, controlCohortId}, cohortPairs)
 	if !validAccessRequest {
@@ -304,7 +303,7 @@ func (u CohortDataController) RetrieveCohortOverlapStats(c *gin.Context) {
 		return
 	}
 	overlapStats, err := u.cohortDataModel.RetrieveCohortOverlapStats(sourceId, caseCohortId,
-		controlCohortId, conceptIds, cohortPairs)
+		controlCohortId, conceptDefsAndCohortPairs)
 	if err != nil {
 		c.JSON(http.StatusInternalServerError, gin.H{"message": "Error retrieving stats", "error": err.Error()})
 		c.Abort()

models/cohortdata.go

@@ -10,7 +10,7 @@ import (
 
 type CohortDataI interface {
 	RetrieveDataBySourceIdAndCohortIdAndConceptIdsOrderedByPersonId(sourceId int, cohortDefinitionId int, conceptIds []int64) ([]*PersonConceptAndValue, error)
-	RetrieveCohortOverlapStats(sourceId int, caseCohortId int, controlCohortId int, otherFilterConceptIds []int64, filterCohortPairs []utils.CustomDichotomousVariableDef) (CohortOverlapStats, error)
+	RetrieveCohortOverlapStats(sourceId int, caseCohortId int, controlCohortId int, filterConceptDefsAndCohortPairs []interface{}) (CohortOverlapStats, error)
 	RetrieveDataByOriginalCohortAndNewCohort(sourceId int, originalCohortDefinitionId int, cohortDefinitionId int) ([]*PersonIdAndCohort, error)
 	RetrieveHistogramDataBySourceIdAndCohortIdAndConceptDefsPlusCohortPairs(sourceId int, cohortDefinitionId int, filterConceptDefsAndCohortPairs []interface{}) ([]*PersonConceptAndValue, error)
 	RetrieveBarGraphDataBySourceIdAndCohortIdAndConceptIds(sourceId int, conceptId int64) ([]*NominalGroupData, error)
@@ -170,27 +170,28 @@ func (h CohortData) RetrieveBarGraphDataBySourceIdAndCohortIdAndConceptIds(sourc
 	return cohortData, meta_result.Error
 }
 
-// Basically the same as the method above, but without the extra filtering on filterConceptId and filterConceptValue:
 func (h CohortData) RetrieveCohortOverlapStats(sourceId int, caseCohortId int, controlCohortId int,
-	filterConceptIds []int64, filterCohortPairs []utils.CustomDichotomousVariableDef) (CohortOverlapStats, error) {
+	filterConceptDefsAndCohortPairs []interface{}) (CohortOverlapStats, error) {
 
 	var dataSourceModel = new(Source)
 	omopDataSource := dataSourceModel.GetDataSource(sourceId, Omop)
 	resultsDataSource := dataSourceModel.GetDataSource(sourceId, Results)
 
 	var cohortOverlapStats CohortOverlapStats
-	query := QueryFilterByCohortPairsHelper(filterCohortPairs, resultsDataSource, caseCohortId, "case_cohort_unionedAndIntersectedWithFilters").
-		Select("count(distinct(case_cohort_unionedAndIntersectedWithFilters.subject_id)) as case_control_overlap").
-		Joins("INNER JOIN " + resultsDataSource.Schema + ".cohort as control_cohort ON control_cohort.subject_id = case_cohort_unionedAndIntersectedWithFilters.subject_id") // this one allows for the intersection between case and control and the assessment of the overlap
+	session := resultsDataSource.Db.Session(&gorm.Session{})
+	err := session.Transaction(func(query *gorm.DB) error {
+		finalSetAlias := "case_cohort_unionedAndIntersectedWithFilters"
+		query, _ = QueryFilterByConceptDefsPlusCohortPairsHelper(query, sourceId, caseCohortId, filterConceptDefsAndCohortPairs, omopDataSource, resultsDataSource, finalSetAlias)
 
-	if len(filterConceptIds) > 0 {
-		query = QueryFilterByConceptIdsHelper(query, sourceId, filterConceptIds, omopDataSource, resultsDataSource.Schema, "control_cohort.subject_id")
-	}
-	query = query.Where("control_cohort.cohort_definition_id = ?", controlCohortId)
-	query, cancel := utils.AddTimeoutToQuery(query)
-	defer cancel()
-	meta_result := query.Scan(&cohortOverlapStats)
-	return cohortOverlapStats, meta_result.Error
+		query = query.Select("count(distinct(case_cohort_unionedAndIntersectedWithFilters.subject_id)) as case_control_overlap").
+			Joins("INNER JOIN "+resultsDataSource.Schema+".cohort as control_cohort ON control_cohort.subject_id = case_cohort_unionedAndIntersectedWithFilters.subject_id"). // this one allows for the intersection between case and control and the assessment of the overlap
+			Where("control_cohort.cohort_definition_id = ?", controlCohortId)
+		query, cancel := utils.AddTimeoutToQuery(query)
+		defer cancel()
+		meta_result := query.Scan(&cohortOverlapStats)
+		return meta_result.Error
+	})
+	return cohortOverlapStats, err
 }
 
 func (p *PersonConceptAndCount) String() string {

models/concept.go

@@ -185,11 +185,11 @@ func (h Concept) RetrieveBreakdownStatsBySourceIdAndCohortIdAndConceptDefsPlusCo
 	var conceptBreakdownList []*ConceptBreakdown
 	session := resultsDataSource.Db.Session(&gorm.Session{})
 	err := session.Transaction(func(query *gorm.DB) error {
-		query, tmpTableName := QueryFilterByConceptDefsPlusCohortPairsHelper(query, sourceId, cohortDefinitionId, filterConceptDefsAndCohortPairs, omopDataSource, resultsDataSource, finalSetAlias)
+		query, finalObservationTableAlias := QueryFilterByConceptDefsPlusCohortPairsHelper(query, sourceId, cohortDefinitionId, filterConceptDefsAndCohortPairs, omopDataSource, resultsDataSource, finalSetAlias)
 		// count persons, grouping by concept value:
-		if tmpTableName != "" {
-			query = query.Select(tmpTableName + ".value_as_concept_id), count(distinct(" + tmpTableName + ".person_id)) as npersons_in_cohort_with_value").
-				Group(tmpTableName + ".value_as_concept_id")
+		if finalObservationTableAlias != "" {
+			query = query.Select(finalObservationTableAlias + ".value_as_concept_id, count(distinct(" + finalObservationTableAlias + ".person_id)) as npersons_in_cohort_with_value").
+				Group(finalObservationTableAlias + ".value_as_concept_id")
 		} else {
 			query = query.Select("observation.value_as_concept_id, count(distinct(observation.person_id)) as npersons_in_cohort_with_value").
 				Joins("INNER JOIN "+omopDataSource.Schema+".observation_continuous as observation"+omopDataSource.GetViewDirective()+" ON "+finalSetAlias+".subject_id = observation.person_id").

models/helper.go

@@ -21,27 +21,31 @@ func QueryFilterByConceptDefsPlusCohortPairsHelper(query *gorm.DB, sourceId int,
 	// Caching of temporary tables: for optimal performance, a temporary table dictionary / cache is updated, keeping a mapping
 	// of existing temporary table names vs underlying subsets of items in filterConceptDefsAndCohortPairs that gave rise to these
 	// tables.
+	// Returns: the final query,
+	// and, if the last variable in filterConceptDefsAndCohortPairs is a concept/numeric one,
+	// this method also returns the final observation table alias representing the filter
+	// added for this variable to the final query.
 	query = query.Table(resultsDataSource.Schema+".cohort as "+finalCohortAlias).
 		Select("*").
-		Where("cohort_definition_id=?", mainCohortDefinitionId)
+		Where(finalCohortAlias+".cohort_definition_id=?", mainCohortDefinitionId)
 
-	tmpTableName := ""
+	finalObservationTableAlias := ""
 	var err error
 	for i, item := range filterConceptDefsAndCohortPairs {
 		observationTableAlias := fmt.Sprintf("filter_%d", i)
 		switch convertedItem := item.(type) {
 		case utils.CustomConceptVariableDef:
-			query, tmpTableName, err = QueryFilterByConceptDefHelper(query, sourceId, convertedItem, omopDataSource, finalCohortAlias, observationTableAlias)
+			query, finalObservationTableAlias, err = QueryFilterByConceptDefHelper(query, sourceId, convertedItem, omopDataSource, finalCohortAlias, observationTableAlias)
 		case utils.CustomDichotomousVariableDef:
 			query = QueryFilterByCohortPairHelper(query, convertedItem, resultsDataSource, mainCohortDefinitionId, finalCohortAlias+".subject_id", observationTableAlias)
-			tmpTableName = ""
+			finalObservationTableAlias = ""
 		}
 	}
 	if err != nil {
 		log.Fatalf("Error: %s", err.Error())
 		panic("error")
 	}
-	return query, tmpTableName
+	return query, finalObservationTableAlias
 }
 
 // DEPRECATED - USE QueryFilterByConceptDefsHelper
@@ -91,7 +95,7 @@ func QueryFilterByConceptDefHelper(query *gorm.DB, sourceId int, filterConceptDe
 		// simple filterConceptDef with no transformation
 		query := queryJoinAndFilterByConceptDef(query, sourceId, filterConceptDef,
 			omopDataSource, personIdFieldForObservationJoin, "observation_continuous", observationTableAlias)
-		return query, "", nil
+		return query, observationTableAlias, nil
 	}
 }
 

tests/controllers_tests/controllers_test.go

@@ -92,7 +92,7 @@ func (h dummyCohortDataModel) RetrieveBarGraphDataBySourceIdAndCohortIdAndConcep
 }
 
 func (h dummyCohortDataModel) RetrieveCohortOverlapStats(sourceId int, caseCohortId int, controlCohortId int,
-	otherFilterConceptIds []int64, filterCohortPairs []utils.CustomDichotomousVariableDef) (models.CohortOverlapStats, error) {
+	filterConceptDefsAndCohortPairs []interface{}) (models.CohortOverlapStats, error) {
 	var zeroOverlap models.CohortOverlapStats
 	return zeroOverlap, nil
 }

tests/models_tests/models_test.go

@@ -475,11 +475,11 @@ func TestRetrieveBreakdownStatsBySourceIdAndCohortIdAndConceptDefsPlusCohortPair
 			ProvidedName:        "test"},
 	}
 	breakdownConceptId := hareConceptId // not normally the case...but we'll use the same here just for the test...
-	stats, _ := conceptModel.RetrieveBreakdownStatsBySourceIdAndCohortIdAndConceptDefsPlusCohortPairs(testSourceId,
+	stats, err := conceptModel.RetrieveBreakdownStatsBySourceIdAndCohortIdAndConceptDefsPlusCohortPairs(testSourceId,
 		extendedCopyOfSecondLargestCohort.Id, filterConceptDefsAndCohortPairs, breakdownConceptId)
 	// we expect values since secondLargestCohort has multiple subjects with hare info:
 	if len(stats) < 4 {
-		t.Errorf("Expected at least 4 results, found %d", len(stats))
+		t.Errorf("Expected at least 4 results, found %d. Error: %v", len(stats), err)
 	}
 	prevName := ""
 	for _, stat := range stats {
@@ -499,11 +499,11 @@ func TestRetrieveBreakdownStatsBySourceIdAndCohortIdAndConceptDefsPlusCohortPair
 			ConceptId: hareConceptId,
 		},
 	}
-	stats2, _ := conceptModel.RetrieveBreakdownStatsBySourceIdAndCohortIdAndConceptDefsPlusCohortPairs(testSourceId,
+	stats2, err := conceptModel.RetrieveBreakdownStatsBySourceIdAndCohortIdAndConceptDefsPlusCohortPairs(testSourceId,
 		extendedCopyOfSecondLargestCohort.Id, filterConceptDefsAndCohortPairs, breakdownConceptId)
 	// very rough check (ideally we would check the individual stats as well...TODO?):
 	if len(stats) > len(stats2) {
-		t.Errorf("First query is more restrictive, so its stats should not be larger than stats2 of second query. Got %d and %d", len(stats), len(stats2))
+		t.Errorf("First query is more restrictive, so its stats should not be larger than stats2 of second query. Got %d and %d. Error: %v", len(stats), len(stats2), err)
 	}
 
 	// test filtering with secondLargestCohort, smallest and largestCohort.
@@ -1342,53 +1342,63 @@ func TestRetrieveCohortOverlapStats(t *testing.T) {
 	setUp(t)
 	caseCohortId := secondLargestCohort.Id
 	controlCohortId := secondLargestCohort.Id // to ensure we get some overlap, just repeat the same here...
-	otherFilterConceptIds := []int64{}
-	filterCohortPairs := []utils.CustomDichotomousVariableDef{}
-	stats, _ := cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
-		otherFilterConceptIds, filterCohortPairs)
+	filterConceptDefsAndCohortPairs := []interface{}{}
+	stats, err := cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
+		filterConceptDefsAndCohortPairs)
 	// basic test:
 	if stats.CaseControlOverlap != int64(secondLargestCohort.CohortSize) {
-		t.Errorf("Expected nr persons to be %d, found %d", secondLargestCohort.CohortSize, stats.CaseControlOverlap)
+		t.Errorf("Expected nr persons to be %d, found %d. Error: %v", secondLargestCohort.CohortSize, stats.CaseControlOverlap, err)
 	}
 
 	// now use largestCohort as background and filter on the extendedCopyOfSecondLargestCohort
 	caseCohortId = largestCohort.Id
 	controlCohortId = largestCohort.Id // to ensure we get largestCohort as initial overlap, just repeat the same here...
-	filterCohortPairs = []utils.CustomDichotomousVariableDef{
-		{
+	filterConceptDefsAndCohortPairs = []interface{}{
+		utils.CustomDichotomousVariableDef{
 			CohortDefinitionId1: smallestCohort.Id,
 			CohortDefinitionId2: extendedCopyOfSecondLargestCohort.Id,
-			ProvidedName:        "test"},
+			ProvidedName:        "test",
+		},
 	}
 	// then we expect overlap of 6 for extendedCopyOfSecondLargestCohort and largestCohort:
-	stats, _ = cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
-		otherFilterConceptIds, filterCohortPairs)
+	stats, err = cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
+		filterConceptDefsAndCohortPairs)
 	if stats.CaseControlOverlap != 6 {
-		t.Errorf("Expected nr persons to be %d, found %d", 6, stats.CaseControlOverlap)
+		t.Errorf("Expected nr persons to be %d, found %d. Error: %v", 6, stats.CaseControlOverlap, err)
 	}
 
 	// extra test: different parameters that should return the same as above ^:
 	caseCohortId = largestCohort.Id
 	controlCohortId = extendedCopyOfSecondLargestCohort.Id
-	filterCohortPairs = []utils.CustomDichotomousVariableDef{}
-	otherFilterConceptIds = []int64{histogramConceptId} // extra filter, to cover this part of the code...
+	filterConceptDefsAndCohortPairs = []interface{}{
+		utils.CustomConceptVariableDef{
+			ConceptId: histogramConceptId,
+		},
+	} // extra filter, to cover this part of the code...
 	// then we expect overlap of 5 for extendedCopyOfSecondLargestCohort and largestCohort (the filter on histogramConceptId should not matter
 	// since all in largestCohort have an observation for this concept id except one person who has it but has value_as_number as NULL):
-	stats2, _ := cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
-		otherFilterConceptIds, filterCohortPairs)
+	stats2, err := cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
+		filterConceptDefsAndCohortPairs)
 	if stats2.CaseControlOverlap != stats.CaseControlOverlap-1 {
-		t.Errorf("Expected nr persons to be %d, found %d", stats.CaseControlOverlap, stats2.CaseControlOverlap)
+		t.Errorf("Expected nr persons to be %d, found. %d Error: %v", stats.CaseControlOverlap, stats2.CaseControlOverlap, err)
 	}
 
 	// test for otherFilterConceptIds by filtering above on dummyContinuousConceptId, which is NOT
 	// found in any observations of the largestCohort:
-	otherFilterConceptIds = []int64{histogramConceptId, dummyContinuousConceptId}
+	filterConceptDefsAndCohortPairs = []interface{}{
+		utils.CustomConceptVariableDef{
+			ConceptId: histogramConceptId,
+		},
+		utils.CustomConceptVariableDef{
+			ConceptId: dummyContinuousConceptId,
+		},
+	}
 	// all other arguments are the same as test above, and we expect overlap of 0, showing the otherFilterConceptIds
 	// had the expected effect:
-	stats3, _ := cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
-		otherFilterConceptIds, filterCohortPairs)
+	stats3, err := cohortDataModel.RetrieveCohortOverlapStats(testSourceId, caseCohortId, controlCohortId,
+		filterConceptDefsAndCohortPairs)
 	if stats3.CaseControlOverlap != 0 {
-		t.Errorf("Expected nr persons to be 0, found %d", stats3.CaseControlOverlap)
+		t.Errorf("Expected nr persons to be 0, found %d. Error: %v", stats3.CaseControlOverlap, err)
 	}
 }