Add demographic breakdown to recidivism chart #238

macfarlandian · 2020-10-28T19:13:42Z

Description of the change

When a single cohort is selected, enables the standard demographic breakdown as seen in the rest of the site. When this happens the chart displays one line per demographic group; when multiple cohorts are selected, this control is disabled and the view reverts to the total for that cohort.

@jjdressel11 please let me know if these fixture files do not match how you'd like the real exports to look! (The values in them, FYI, are completely random, though I tried to make curves resemble the exponential decay we have observed in real data)

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #233

Checklists

Development

These boxes should be checked by the submitter prior to merging:

Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

This pull request has a descriptive title and information useful to a reviewer
This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
Potential security implications or infrastructural changes have been considered, if relevant

coveralls · 2020-10-28T19:16:11Z

Pull Request Test Coverage Report for Build 338364154

29 of 41 (70.73%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+9.2%) to 25.408%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
public-dashboard-client/src/testUtils.js	10	11	90.91%
public-dashboard-client/src/viz-recidivism-rates/VizRecidivismRates.js	13	14	92.86%
public-dashboard-client/src/page-sentencing/PageSentencing.js	0	5	0.0%
public-dashboard-client/src/viz-recidivism-rates/RecidivismRatesChart.js	5	10	50.0%

Files with Coverage Reduction	New Missed Lines	%
public-dashboard-client/src/page-sentencing/PageSentencing.js	1	0%

Totals
Change from base Build 338343730:	9.2%
Covered Lines:	393
Relevant Lines:	1492

💛 - Coveralls

jessex

Looks and functions great, @macfarlandian!

jovergaag

Suggestion for a place to add a test, plus a few missing copyright disclaimers. This is AWESOME!

jovergaag · 2020-10-28T21:37:41Z

public-dashboard-client/src/controls/DimensionControl.js

@@ -3,9 +3,14 @@ import PropTypes from "prop-types";
 import Dropdown from "./Dropdown";
 import { DIMENSIONS_LIST } from "../constants";

-export default function DimensionControl({ onChange }) {
+export default function DimensionControl({ onChange, ...passThruProps }) {


I've just noticed that this file and a few others that you've touched here do not have the Copyright disclaimer at the top.

jovergaag · 2020-10-28T22:09:47Z

public-dashboard-client/src/viz-recidivism-rates/RecidivismRatesChart.js

@@ -136,7 +137,10 @@ export default function RecidivismRatesChart({ data, highlightedCohort }) {
                }}
                renderKey={(d) =>
                  // if it has a label, it's a line; if not, it's a point
-                  d.label || `${d.releaseCohort}-${d.followupYears}`
+                  d.label ||
+                  `${d.releaseCohort}-${d[DIMENSION_DATA_KEYS.race]}${


I finally figured out what you mean by lines and points. They are the lines and points passed into the XYFrame.
Maybe it would be more clear to add a label to each point on line 62 when they are created? Or in prepareChartData. That may be a little more clear how the points are going to be labeled and avoid this confusion. Just a thought.

yes great suggestion!

jovergaag · 2020-10-28T22:16:17Z

public-dashboard-client/src/viz-recidivism-rates/VizRecidivismRates.js

+ * Otherwise (i.e., a single cohort and some dimensional breakdown is selected),
+ * will return one data series per demographic subgroup.
+ */
+function prepareChartData({ data, dimension, selectedCohorts }) {


This might be a nice place to add a test. There are a few branches of logic so a couple of tests to show the chart data at the end of each branch might be helpful.

I did add some tests here but I have mixed feelings about them. Exporting and testing this function itself felt like too much of a low-level implementation detail, since it's so specific to the Semiotic API. Testing the rendered output is also not super obvious since it's mainly just, like, drawing a bunch of lines? I settled on testing the label text that Semiotic renders, since it should reflect the data input in the same way that the chart marks do, but it's still pretty idiosyncratic ... e.g. if we swapped in a different library that produced an equivalent-looking chart I have low confidence that these tests would still pass, since chart labeling is still not a well established web standard.

Also I had to do something kind of funky to get the data fixture into the test environment? I actually think it's kind of a fun solution honestly but it is definitely quick-and-dirty.

…ographics

…r key

jovergaag

Awesome. Just one comment about the fixture data. 🌞

jovergaag · 2020-10-30T19:25:46Z

public-dashboard-client/src/testUtils.js

@@ -20,3 +44,26 @@ export { customRender as render };

 // provide original render method as a fallback if needed
 export { render as renderUnwrapped };
+
+// retrieve a data fixture from spotlight-api
+export function getDataFixture(filename) {


Oh thats interesting. Is this fixture data manually generated or part of the nightly build and randomized? Just want to keep our eye on when/how this fixture data will become out of sync with live data and what that might mean for the tests.

we have just been manually generating them when a new file is added to the API. I think it mostly doesn't matter if the values drift a bit, but if the shape drifts and causes tests to fail then that's a good signal that we need to update our fixtures anyway?

The one exception to this I can think of is the time-series metrics (population, supervision success rates), which have some baked-in assumptions about the data being relative to the current date. There is already a test for some of that functionality (a utility function that handles missing data) that mocks the system date to create different test conditions, so probably we would just have to do some more of that if we wanted to use those fixtures for something.

jjdressel11

Besides moving this to the Prison page, this looks great!

jjdressel11 · 2020-10-30T20:09:22Z

spotlight-api/core/demo_data/recidivism_rates_by_cohort_by_year.json

-{"state_code": "US_DEMO", "release_cohort": "2017", "followup_years": "1", "recidivism_rate": "0.28022799575391744", "recidivated_releases": "83", "releases": "297"}
-{"state_code": "US_DEMO", "release_cohort": "2017", "followup_years": "2", "recidivism_rate": "0.3273667202949975", "recidivated_releases": "97", "releases": "297"}
-{"state_code": "US_DEMO", "release_cohort": "2018", "followup_years": "1", "recidivism_rate": "0.14163508588973228", "recidivated_releases": "45", "releases": "322"}
+{"state_code": "US_DEMO", "gender": "ALL", "age_bucket": "ALL", "race_or_ethnicity": "ALL", "releases": 441, "release_cohort": "2009", "followup_years": "1", "recidivism_rate": "0.15472315694153785", "recidivated_releases": "68"}


My backend support query produces a file with this name and with these fields!

jjdressel11 · 2020-10-30T20:10:14Z

public-dashboard-client/src/page-sentencing/PageSentencing.js

  const [highlightedCohort, setHighlightedCohort] = useState();
+  const [recidivismDimension, setRecidivismDimension] = useState(


Agreed offline that these charts should be on the Prison page instaed

yes I am going to deal with that in a separate PR. Thanks for catching it!

macfarlandian added 17 commits October 22, 2020 17:15

Cohort select functionality (unstyled)

65739c9

tests for dropdown before refactoring

3e76281

fix excessive z-index in chart overlay

48a0e4e

menu UI placement and styling

e75971c

XHoverController should pass through otherChartProps

de6b235

connect cohort control to chart

46968a0

proper button cursor

ad68d74

cleanup and comments

9d33c30

fix typing error in spotlight tests

ed37a07

add a select all option

a706efb

use native select element on mobile

05a5cbb

test mobile menu interaction

15a2b43

update demo data with demographic breakdowns

c20e29d

enable dimension control for single selected cohort

c06be24

pass demographic breakdown to chart when active

fb03070

unique render keys for demographic line points

d878e2d

fix lint error

3dc5fbd

macfarlandian requested review from jessex, daschi, jovergaag and jjdressel11 October 28, 2020 19:13

select all clears selection when all are selected

eeb4680

jessex approved these changes Oct 28, 2020

View reviewed changes

jessex mentioned this pull request Oct 28, 2020

Add cohort filter to recidivism chart #237

Merged

8 tasks

jovergaag requested changes Oct 28, 2020

View reviewed changes

macfarlandian added 4 commits October 28, 2020 16:14

Merge branch 'master' into ian/233-recidivism-filters

dd73e81

vary colors in cohort select test

3bd8b3b

somehow I introduced a gremlin while resolving a merge conflict!

4ca77be

Merge branch 'ian/233-recidivism-filters' into ian/233-recidivism-dem…

2fe23c7

…ographics

macfarlandian added 5 commits October 28, 2020 16:56

add license snippet to all touched files

20fa41c

make sure all points and lines have a "key" property for use as rende…

cd06b52

…r key

add some basic chart tests

d241289

remove commented out code

fa88dbd

fix z-index issue with multiple menus

71418f5

jovergaag approved these changes Oct 30, 2020

View reviewed changes

jjdressel11 approved these changes Oct 30, 2020

View reviewed changes

Base automatically changed from ian/233-recidivism-filters to master October 30, 2020 20:48

macfarlandian added 2 commits October 30, 2020 14:04

Merge branch 'master' into ian/233-recidivism-demographics

e28efed

remove duplicate code from merge conflict

e05a373

macfarlandian merged commit 739a0aa into master Oct 30, 2020

macfarlandian deleted the ian/233-recidivism-demographics branch October 30, 2020 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add demographic breakdown to recidivism chart #238

Add demographic breakdown to recidivism chart #238

macfarlandian commented Oct 28, 2020 •

edited by jovergaag

coveralls commented Oct 28, 2020 •

edited

jessex left a comment

jovergaag left a comment

jovergaag Oct 28, 2020

jovergaag Oct 28, 2020

macfarlandian Oct 28, 2020

jovergaag Oct 28, 2020

macfarlandian Oct 29, 2020

jovergaag left a comment

jovergaag Oct 30, 2020

macfarlandian Oct 30, 2020

jjdressel11 left a comment

jjdressel11 Oct 30, 2020

jjdressel11 Oct 30, 2020

macfarlandian Oct 30, 2020

		const [highlightedCohort, setHighlightedCohort] = useState();
		const [recidivismDimension, setRecidivismDimension] = useState(

Add demographic breakdown to recidivism chart #238

Add demographic breakdown to recidivism chart #238

Conversation

macfarlandian commented Oct 28, 2020 • edited by jovergaag

Description of the change

Type of change

Related issues

Checklists

Development

Code review

coveralls commented Oct 28, 2020 • edited

Pull Request Test Coverage Report for Build 338364154

💛 - Coveralls

jessex left a comment

Choose a reason for hiding this comment

jovergaag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovergaag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjdressel11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macfarlandian commented Oct 28, 2020 •

edited by jovergaag

coveralls commented Oct 28, 2020 •

edited