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

Create basic version of census summarization #20357

Merged
merged 3 commits into from Feb 1, 2018
Merged

Conversation

drewsamnick
Copy link
Contributor

Creates an initial version of census summarization logic which counts any inconsistent data as a MAYBE. This logic will change to something more complex soon but we want to populate the summary table to make it easier to build out visualizations. For now I am adding a one-off script to run this code, to be moved to a cron job once we have a more finalized version of the logic in place.

I also needed to add in some reverse relationships in the models to be able to get from a School to the various census data. From my testing I do not believe that this will impact anything since those relations aren't loaded by default.

@drewsamnick drewsamnick requested a review from aoby February 1, 2018 20:42
@drewsamnick
Copy link
Contributor Author

@@ -28,4 +28,161 @@ class Census::CensusSummary < ApplicationRecord
enum teaches_cs: TEACHES

validates_presence_of :audit_data

def self.submission_teaches_cs(submission, is_high_school)
Copy link
Contributor

Choose a reason for hiding this comment

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

how about making this a question mark method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll make that change.

@@ -28,4 +28,161 @@ class Census::CensusSummary < ApplicationRecord
enum teaches_cs: TEACHES

validates_presence_of :audit_data

def self.submission_teaches_cs(submission, is_high_school)
if is_high_school
Copy link
Contributor

Choose a reason for hiding this comment

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

And this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_high_school is an argument to the method, not a method itself. I didn't think that variable names typically ended with a question mark.

Copy link
Contributor

Choose a reason for hiding this comment

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

No they don't, and actually can't. If it's a variable, don't worry about it. I thought it was another method call.

where(schools: {state: state}).
select(:school_year).
group(:school_year).
map(&:school_year)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does pluck(:school_year) work here instead of map? Since we're only selecting that column it probably makes little to no difference so not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both work and generate the exact same SQL.

latest_survey_year,
latest_ap_data_year,
latest_ib_data_year,
latest_state_data_years.map {|_, v| v}.max
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do? AFAIK map for an array will only provide a single parameter to the block. Are you missing a key, or is this meant to be something else?

irb(main):001:0> [1,2].map{|_,v| v}
=> [nil, nil]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

latest_state_data_years is a hash, not any array. The keys are the state code and the value is the latest year that has data for that state. This is taking the latest year across all states that have data. This would be clearer if I rename latest_state_data_years to latest_data_year_by_state and then do latest_data_year_by_state.values.max

Copy link
Contributor

Choose a reason for hiding this comment

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

OIC. That makes sense. I misread that anyway and thought the map was outside the array. Maybe add a small comment (or rename as you suggested).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cleaned this up a bit. Is it clearer now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better. Thanks!

yes_count = 0
no_count = 0

# If the schools doesn't have stats then treat it as not high school.
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar nit: school doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@drewsamnick drewsamnick merged commit de9e8b4 into staging Feb 1, 2018
@drewsamnick drewsamnick deleted the census-summarization branch February 1, 2018 23:08
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.

None yet

2 participants