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

US NHTSA FARS Crash NL #4642

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

Conversation

saanikaaa
Copy link

@saanikaaa saanikaaa commented Sep 25, 2024

@saanikaaa
Copy link
Author

/gcbrun

@hareesh-ms
Copy link

Is the step to update the integration golden files executed by running ./run_test.sh -g ?

@HarishC727
Copy link

/gcbrun

@hareesh-ms
Copy link

/gcbrun

Copy link
Contributor

@chejennifer chejennifer left a 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
Copy link
Contributor

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:

  1. 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.
  2. 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.
  3. Also add a test query that triggers the new stat var

Copy link
Author

Choose a reason for hiding this comment

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

  1. Restrict the addition to less than 10.
  2. Modified to one description. Updated "accident" over "crash".
  3. Added test query.

@HarishC727
Copy link

/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
Copy link
Contributor

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"

Copy link
Author

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"

@hareesh-ms
Copy link

/gcbrun

Copy link
Contributor

@chejennifer chejennifer left a 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

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

Successfully merging this pull request may close these issues.

5 participants