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.

Sign up for GitHub

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

Jump to bottom

Add tests for calculated obsCollection #1373

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add tests for calculated obsCollection #1373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5a582e
fix
n-h-diaz May 31, 2024
fc5c444
Add tests for calculated obsCollection
n-h-diaz Jun 1, 2024
ada73fd
fix
n-h-diaz Jun 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions internal/server/v2/observation/contained_in.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,11 @@ func FetchContainedIn(
var err error
if store.BtGroup != nil {
readCollectionCache := false
childPlaces, err = FetchChildPlaces(
ctx, store, metadata, httpClient, remoteMixer, ancestor, childType)
if err != nil {
return nil, err
}
Comment on lines +189 to +193
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to always fetch this since the obsCollection collection could already exists from BT cache.

if queryDate != "" {
result = &pbv2.ObservationResponse{
ByVariable: map[string]*pbv2.VariableObservation{},
Expand All @@ -210,6 +215,10 @@ func FetchContainedIn(
obsByEntity := result.ByVariable[variable].ByEntity
data, ok := btData[variable]
if !ok || data == nil {
// If data is missing, attach empty child places.
for _, place := range childPlaces {
obsByEntity[place] = &pbv2.EntityObservation{}
}
Comment on lines +218 to +221
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels like this should be part of Flume logic. Right now Flume does not return place with no data. This is a behavior discrepancy.

I actually think it makes sense to not have an entry for a child place if there is no data in all scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add this to the flume instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think we should add places with no data. Imagine fetching cities in USA for a custom DC variable which has data for 10 places. That would have thousands of empty entries.

I think we should gvc and understand why the child places are needed..

continue
}
cohorts := data.SourceCohorts
Expand Down Expand Up @@ -257,11 +266,6 @@ func FetchContainedIn(
}
}
if !readCollectionCache {
childPlaces, err = FetchChildPlaces(
ctx, store, metadata, httpClient, remoteMixer, ancestor, childType)
if err != nil {
return nil, err
}
totalSeries := len(variables) * len(childPlaces)
if totalSeries > MaxSeries {
return nil, status.Errorf(
Expand Down
225 changes: 225 additions & 0 deletions internal/server/v2/observation/golden/calculation/contained_in_2015.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
{
"by_variable": {
"Count_Person_Female": {
"by_entity": {
"wikidataId/Q159477": {
"ordered_facets": [
{
"observations": [
{
"date": "1975",
"value": 6170
},
{
"date": "1980",
"value": 13856
},
{
"date": "1985",
"value": 21711
},
{
"date": "1995",
"value": 48659
},
{
"date": "2005",
"value": 75313
}
],
"obs_count": 5,
"earliest_date": "1975",
"latest_date": "2005"
}
]
},
"wikidataId/Q170024": {
"ordered_facets": [
{
"observations": [
{
"date": "1975",
"value": 17307
},
{
"date": "1980",
"value": 27393
},
{
"date": "1985",
"value": 40165
},
{
"date": "1995",
"value": 59342
},
{
"date": "2005",
"value": 80621
}
],
"obs_count": 5,
"earliest_date": "1975",
"latest_date": "2005"
}
]
},
"wikidataId/Q175021": {
"ordered_facets": [
{
"observations": [
{
"date": "1975",
"value": 2645
},
{
"date": "1980",
"value": 4466
},
{
"date": "1985",
"value": 7626
},
{
"date": "1995",
"value": 14414
},
{
"date": "2005",
"value": 19075
}
],
"obs_count": 5,
"earliest_date": "1975",
"latest_date": "2005"
}
]
},
"wikidataId/Q187712": {
"ordered_facets": [
{
"observations": [
{
"date": "1975",
"value": 56754
},
{
"date": "1980",
"value": 120160
},
{
"date": "1985",
"value": 185783
},
{
"date": "1995",
"value": 291719
},
{
"date": "2005",
"value": 472665
}
],
"obs_count": 5,
"earliest_date": "1975",
"latest_date": "2005"
}
]
},
"wikidataId/Q188810": {
"ordered_facets": [
{
"observations": [
{
"date": "1975",
"value": 27535
},
{
"date": "1980",
"value": 56532
},
{
"date": "1985",
"value": 86466
},
{
"date": "1995",
"value": 148478
},
{
"date": "2005",
"value": 273339
}
],
"obs_count": 5,
"earliest_date": "1975",
"latest_date": "2005"
}
]
},
"wikidataId/Q4091": {
"ordered_facets": [
{
"observations": [
{
"date": "1975",
"value": 6683
},
{
"date": "1980",
"value": 11393
},
{
"date": "1985",
"value": 18343
},
{
"date": "1995",
"value": 30414
},
{
"date": "2005",
"value": 47114
}
],
"obs_count": 5,
"earliest_date": "1975",
"latest_date": "2005"
}
]
},
"wikidataId/Q613": {
"ordered_facets": [
{
"observations": [
{
"date": "1975",
"value": 54366
},
{
"date": "1980",
"value": 88587
},
{
"date": "1985",
"value": 123609
},
{
"date": "1995",
"value": 211211
},
{
"date": "2005",
"value": 332148
}
],
"obs_count": 5,
"earliest_date": "1975",
"latest_date": "2005"
}
]
}
}
}
}
}
67 changes: 67 additions & 0 deletions internal/server/v2/observation/golden/calculation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,70 @@ func TestCalculation(t *testing.T) {
t.Errorf("TestDriver() = %s", err)
}
}

func TestCalculationForObsCollection(t *testing.T) {
t.Parallel()
ctx := context.Background()

_, filename, _, _ := runtime.Caller(0)
goldenPath := path.Join(path.Dir(filename), "calculation")

testSuite := func(mixer pbs.MixerClient, latencyTest bool) {
for _, c := range []struct {
desc string
variables []string
entityExpression string
date string
filter *pbv2.FacetFilter
goldenFile string
}{
{
"contained in 2015",
[]string{
"Count_Person_Female",
},
"country/ARE<-containedInPlace+{typeOf:AdministrativeArea1}",
"2015",
nil,
"contained_in_2015.json",
},
} {
goldenFile := c.goldenFile
resp, err := mixer.V2Observation(ctx, &pbv2.ObservationRequest{
Select: []string{"variable", "entity", "date", "value"},
Variable: &pbv2.DcidOrExpression{Dcids: c.variables},
Entity: &pbv2.DcidOrExpression{Expression: c.entityExpression},
Date: c.date,
Filter: c.filter,
})
if err != nil {
t.Errorf("could not run V2Observation (direct): %s", err)
continue
}
if latencyTest {
continue
}
if test.GenerateGolden {
test.UpdateGolden(resp, goldenPath, goldenFile)
continue
}
var expected pbv2.ObservationResponse
if err = test.ReadJSON(goldenPath, goldenFile, &expected); err != nil {
t.Errorf("Can not Unmarshal golden file: %s", err)
continue
}
if diff := cmp.Diff(resp, &expected, protocmp.Transform()); diff != "" {
t.Errorf("%s: got diff: %s", c.desc, diff)
continue
}
}
}
if err := test.TestDriver(
"TestCalculationForObsCollection",
&test.TestOption{UseSQLite: true, CacheSVFormula: true},
testSuite,
); err != nil {
t.Errorf("TestDriver() = %s", err)
}

}
53 changes: 52 additions & 1 deletion internal/server/v2/observation/golden/contained_in_2015/US_State.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
"by_variable": {
"test_var_1": {
"by_entity": {
"geoId/01": {},
"geoId/02": {},
"geoId/04": {},
"geoId/05": {},
"geoId/06": {
"ordered_facets": [
{
Expand All @@ -17,7 +21,54 @@
"latest_date": "2015"
}
]
}
},
"geoId/08": {},
"geoId/09": {},
"geoId/10": {},
"geoId/11": {},
"geoId/12": {},
"geoId/13": {},
"geoId/15": {},
"geoId/16": {},
"geoId/17": {},
"geoId/18": {},
"geoId/19": {},
"geoId/20": {},
"geoId/21": {},
"geoId/22": {},
"geoId/23": {},
"geoId/24": {},
"geoId/25": {},
"geoId/26": {},
"geoId/27": {},
"geoId/28": {},
"geoId/29": {},
"geoId/30": {},
"geoId/31": {},
"geoId/32": {},
"geoId/33": {},
"geoId/34": {},
"geoId/35": {},
"geoId/36": {},
"geoId/37": {},
"geoId/38": {},
"geoId/39": {},
"geoId/40": {},
"geoId/41": {},
"geoId/42": {},
"geoId/44": {},
"geoId/45": {},
"geoId/46": {},
"geoId/47": {},
"geoId/48": {},
"geoId/49": {},
"geoId/50": {},
"geoId/51": {},
"geoId/53": {},
"geoId/54": {},
"geoId/55": {},
"geoId/56": {},
"geoId/72": {}
}
}
},
Expand Down
Loading