-
Notifications
You must be signed in to change notification settings - Fork 83
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
US NHTSA FARS Crash NL #4642
base: master
Are you sure you want to change the base?
US NHTSA FARS Crash NL #4642
Conversation
/gcbrun |
Is the step to update the integration golden files executed by running ./run_test.sh -g ? |
/gcbrun |
/gcbrun |
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.
A few comments on the descriptions added:
- we usually want to have 1 good specific description per stat var, maybe a second if necessary
- some of the descriptions with "number of crash" is too vague, we should be more specific that these are vehicle crashes
Count_Person_65OrMoreYears,senior population count | ||
Count_VehicleCrashIncident_CountyRoad,number of crash on county road;number of motor vehicle accident on county road;number of road accident on county road |
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.
A few high level points:
- Only add high level stat vars in this PR and restrict the addition to less than 10. For example,
Count_VehicleCrashIncident
,Count_MortalityEvent_VehicleCrashIncident
are good ones.Count_VehicleCrashIncident_YIntersection
is not a good candidate. - Try add one description for each stat var. Only add additional one if the description states very differently from the primary one. In this example, I would prefer "accident" over "crash". Description like
road accident
is very vague and not accurate. - Also add a test query that triggers the new stat var
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.
- Restrict the addition to less than 10.
- Modified to one description. Updated "accident" over "crash".
- Added test query.
/gcbrun |
Count_VehicleCrashIncident_SleetOrHail,number of vehicle accident in sleet or hail | ||
Count_VehicleCrashIncident_Snow,number of vehicle accident in snow | ||
Count_VehicleCrashIncident_InvolvedInCrash_SchoolBus,number of school bus accident |
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.
to be consistent with the rest of the descriptions, maybe something like "number of vehicle accident with school bus" or "number of vehicle accident involving school bus"
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.
Modified to "number of vehicle accident involving school bus"
/gcbrun |
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.
thanks for updating! The link to the sv differ results seems to be broken, can you please fix that? And please also fix the merge conflicts
SV Index Differ