-
Notifications
You must be signed in to change notification settings - Fork 69
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
Major Index Update: change default index to base_uae_mem #4241
Conversation
server/integration_tests/test_data/demo2_cities_feb2023/query_8/chart_config.json
Outdated
Show resolved
Hide resolved
server/integration_tests/test_data/demo_climatetrace/query_1/chart_config.json
Outdated
Show resolved
Hide resolved
...s/test_data/e2e_answer_places/californiacountieswiththehighestasthmalevels/chart_config.json
Outdated
Show resolved
Hide resolved
server/integration_tests/test_data/demo_feb2023/query_1/chart_config.json
Outdated
Show resolved
Hide resolved
"Count_HousingUnit_HomeValue150000To174999USDollar", | ||
"Count_HousingUnit_HomeValue2000000OrMoreUSDollar", | ||
"Count_HousingUnit_HomeValueUpto10000USDollar" | ||
"dc/topic/HousesByOwnershipCost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff is OK, but just need to check why the topic order flipped compared to dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From debug page, the variables look very different from the golden here.
Query "What is the relationship between housing size and home prices in California".
should the chart_config.json be exact same as what shows up on the page?
..._tests/test_data/detection_api_context/countrieswithgreenhousegasemissions/chart_config.json
Outdated
Show resolved
Hide resolved
..._data/detection_api_context/medianincomeinsantaclaracountyandalamedacounty/chart_config.json
Outdated
Show resolved
Hide resolved
...tests/test_data/detection_api_statvars/incomeininformationindustryinnevada/chart_config.json
Outdated
Show resolved
Hide resolved
server/integration_tests/test_data/detection_translate_chinese/chart_config.json
Outdated
Show resolved
Hide resolved
server/integration_tests/test_data/e2e_correlation_bugs/chart_config.json
Outdated
Show resolved
Hide resolved
...ta/e2e_electrification_demo/whatisthegreenhousegasemissionsfromtheseplaces/chart_config.json
Outdated
Show resolved
Hide resolved
..._tests/test_data/e2e_india_demo/howdoesliteracyratecomparetopovertyinindia/chart_config.json
Show resolved
Hide resolved
...t_data/e2e_sdg_main_dc/compareprogressonpovertyinmexico,nigeriaandpakistan/chart_config.json
Show resolved
Hide resolved
"geoId/06077" | ||
], | ||
"statVarKey": [ | ||
"Count_Person_15OrMoreYears_WithIncome10" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a loss. Currently we have "Household Median Income", which is reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed now
"geoId/06013", | ||
"geoId/06061" | ||
], | ||
"description": "Non-Institutionalized Civilian Adults With No Health Insurance, Earnings 4,999 USD or Less in Los Angeles County", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, household median income on LHS seems more reasonable than this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query string is "income 50000", with the new stat var description, we have $50000 and $5000 exist in the description now.
if you change the query to "Counties in California where income is over 60000", it will be different. So i think the issue here is to handle the numbers in the query?
server/integration_tests/test_data/textbox_sample/query_1/chart_config.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - pending the 4 diff Qs.
@@ -10,27 +10,28 @@ | |||
"tiles": [ | |||
{ | |||
"statVarKey": [ | |||
"Median_Income_Household_HouseholderRaceAsianAlone" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget what we said about this, LHS seems to have income and race, but RHS only race?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query string is "asian population income". There is no exact match for this one but has:
- asian population
- income of asian householder household
- earnings of household with asian born people
Looks like the model prefers to match super set and then match deviating constraints.
@@ -10,27 +10,28 @@ | |||
"tiles": [ | |||
{ | |||
"statVarKey": [ | |||
"Median_Income_Household_HouseholderRaceAsianAlone" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the other "tell me asian california population with low income" issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super excited to get this in!!!
Thanks for review, debugging and all the help! Going to merge now. |
@@ -188,10 +189,7 @@ function update_integration_test_golden { | |||
export LLM_API_KEY= | |||
export ENABLE_EVAL_TOOL=true | |||
|
|||
export ENV_PREFIX=Autopush | |||
python3 -m pytest -vv server/integration_tests/topic_cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already covered in the test below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, yes
This is a major release of the NL feature to replace the all-mini-lm-l6 model with uae-large-v1 that is hosted on vertex AI.
Also made fundamental improvements to stat var descriptions, and only use one accurate description per stat var. This get rid of the need to use alternatives.
This adds some small debug info UI improvement.