Implement Riders narrative #576

nasaownsky · 2022-06-09T13:50:04Z

Description of the change

Implemented base Riders narrative

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)

Closes #518

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 · 2022-06-09T13:57:36Z

Pull Request Test Coverage Report for Build 2711312681

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

61 of 211 (28.91%) changed or added relevant lines in 27 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-5.1%) to 78.516%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
spotlight-client/src/DataStore/TenantStore.ts	1	2	50.0%
spotlight-client/src/OtherNarrativesPageContainer/OtherNarrativesPageContainer.tsx	2	3	66.67%
spotlight-client/src/SiteNavigation/SiteNavigationMobile.tsx	0	1	0.0%
spotlight-client/src/contentModels/RidersNarrative.ts	11	12	91.67%
spotlight-client/src/metricsApi/utils.ts	1	2	50.0%
spotlight-client/src/contentModels/unknowns.ts	1	3	33.33%
spotlight-client/src/metricsApi/HistoricalPopulationByCategoryRecord.ts	0	2	0.0%
spotlight-client/src/metricsApi/RateByCategoryAndDemographicsRecord.ts	0	2	0.0%
spotlight-client/src/contentModels/Metric.ts	0	3	0.0%
spotlight-client/src/OtherNarrativesPageContainer/RidersNarrativePage.tsx	1	6	16.67%

Files with Coverage Reduction	New Missed Lines	%
spotlight-client/src/contentModels/unknowns.ts	1	70.59%

Totals
Change from base Build 2707356951:	-5.1%
Covered Lines:	2135
Relevant Lines:	2557

💛 - Coveralls

nasaownsky · 2022-06-09T13:58:47Z

So, I has implemented Riders configuration with existing charts & metrics. @hobuobi @macfarlandian Is there specific data fixtures for this narrative? If yes, we need only to paste them into the project and correct the metrics in createMetricMapping.ts.

hobuobi · 2022-06-10T04:31:54Z

Hey @nasaownsky – great work! @macfarlandian and I chatted and figured that this work will definitely need more time. As such, I think we can ignore the original Monday deadline and take whatever time is necessary to do it right!

Regarding the charts involved, I think the following are relevant, though I would fall back to @macfarlandian 's guidance.

Population Over Time: The original time series chart expects demographic breakdowns. We should change the way that chart expects data (currently only expects to break down the data by race/age/gender).
Dual Bars: To make this one work, I think we can use a similar data format and chart to the Bubble Chart. We will need to get rid of the "All" case, and use "Rider/Termer" as the primary breakdown instead of race/age/gender categories (usual implementation uses race/age/gender as the primary breakdown instead).
- As an alternative, we could also just implement a section very similar to this one, which breaks down the current population. I would just plug this one in as a first pass. (The Rider and Termer demographics don't very significantly anyway!)
Bubble Chart: Should work perfectly fine with the default chart.
Reincarceration Rates: This is actually a completely new chart! I will spec this out tomorrow or early next week, but you can probably infer the behavior based on the mock.
- The alternative for this is to implement the cumulative reincarceration plot from the existing Spotlight dashboard. We'd plot a line for each of three "cohorts" (Probation, Rider, Termer; instead of all the years), and include data for years 0-3. Race/Age/Gender breakdowns could be activated for each (just like the existing dashboard).

Most of the mock data can still be found here. We should be able to get you CSVs with the actual data very soon (cc @samanthahuff).

macfarlandian · 2022-06-10T16:40:35Z

@nasaownsky I will add some more detail to the ticket with implementation suggestions for the points Humphrey mentioned above, just to limit the noise here. You also may wind up wanting to separate it into multiple pull requests because it's going to be a lot (as I am sure you have already discovered for yourself!)

nasaownsky · 2022-07-04T19:38:51Z

Hey @hobuobi @macfarlandian, happy Independence Day guys! So, I'm adding 1st and 3rd sections of Riders narrative, they all pretty much done. A few nits & questions:

There is no color certainty in the mocks. Is there a list of colors in the right order that I can use? Or the current colors are fine?
There wasn't complete ALL fields in US_ID_Rider_offense file, is it intentional? Because without them the bubbles won't appear. Added them manually for now.
I can't get the unknowns method in the new HistoricalPopulationByCategoryMetric to work, it's mostly the types issue. @macfarlandian maybe you could guide me through that or something? I would appreciate this.
Also, I need proper labels for all the new legends and stuff.

Thanks, guys, and have a great weekend.

nasaownsky · 2022-07-04T19:42:19Z

spotlight-client/src/contentModels/HistoricalPopulationByCategoryMetric.ts

+  // get unknowns(): UnknownsByDate | undefined {
+  //   const { allRecords } = this;
+
+  //   if (!allRecords) return undefined;
+
+  //   const countsByDate = groups(allRecords, (r) => r.date)
+  //     .map(([date, records]) => ({
+  //       date,
+  //       unknowns: countRiderUnknowns(records, (groupedRecords) =>
+  //         sum(groupedRecords, (r) => r.count)
+  //       ),
+  //     }))
+  //     .filter((item) => hasUnknowns(item.unknowns));
+
+  //   return countsByDate.length ? countsByDate : undefined;
+  // }


This unknowns method

@nasaownsky I think you're on the right track in that you probably do need a separate count function because this record type does not include demographic fields like the others. You could just have it accept a HistoricalPopulationByCategoryRecord as input or it could be more generic and accept anything that extends {category: string} or something like that ... it can be a little simpler than the countUnknowns function in that it only has to check for unknowns in one field, the category field, and the return type could probably just be a number? (you could probably give it a more generic name also since there is nothing Rider-specific about this logic.)

Essentially a version of this one-line function is all you really need here ... if you wanted to go a step further you could factor that out into something that will generically check any field for unknowns, but I think it's fine to just copy and update it also since it's so simple.

Does that help? Let me know if there are any more specific questions I can answer to assist with this.

nasaownsky · 2022-07-05T16:27:02Z

@hobuobi Added 2nd section. Also, paying your attention that the data is builded not accurately. For example:
-demographic fields use not standard snake case race_or_ethnicity name, but camel case raceOrEthnicity;
-the age_bucket field uses not standard 70<, but 70+;
-in the current population file there is multiple lines with the same demographic values.

…into nasaownsky/518-riders-narrative

hobuobi · 2022-07-11T14:02:01Z

Thanks Ilya! Apologies for the late response, was out of the office for some family things.

Can you send a screen recording? Having some issues with my local environment (data not loading).
I'll fix the data export files!

nasaownsky · 2022-07-11T18:37:40Z

@hobuobi Sure, here you go: https://www.loom.com/share/5c406cc07a534082a6700c0ea2e32edf . Also, you can try run the App in demo mode by using yarn demo. It starts the App with data, that I had corrected.

@macfarlandian I don't understand why SupervisionSuccessRateMetric tests not passing. I never modified that file, and cannot find the reason for this behavior. Do we need to extra modify something when adding new files to demo_data? Could you take a look at this? Thanks!

hobuobi · 2022-07-12T16:14:53Z

Wow, this is looking AMAZING. Thank you for all your help with this.

Regarding functionality: I think this is actually almost perfect based on the video! My only comments (neither of which should block your PR, but should be addressed before launch):

Strange that the population over time includes decimals. @samanthahuff, sorry for not catching this earlier, but do you know why this might be the case?
Can we still implement the small numbers treatment for the new chart that you've created ("What is the reincarceration rate for Riders?")?

Regarding copy: @ldfielding and I will work on refining this, and it should not block merging the PR (another team member can refine this later if necessary).

macfarlandian · 2022-07-12T18:43:43Z

@macfarlandian I don't understand why SupervisionSuccessRateMetric tests not passing. I never modified that file, and cannot find the reason for this behavior. Do we need to extra modify something when adding new files to demo_data? Could you take a look at this? Thanks!

Nothing is really jumping out at me either. I thought maybe it was objecting to some upstream change but I don't really see what that could be, and the error messages are not very informative ... looks like it will require some good old fashioned debugging

nasaownsky · 2022-07-13T10:05:42Z

Can we still implement the small numbers treatment for the new chart that you've created ("What is the reincarceration rate for Riders?")?

@hobuobi I assume I should use cohort_size of each of the cohort separately and if it's lesser than 100 then display treatment?

nasaownsky · 2022-07-18T14:37:16Z

Can we still implement the small numbers treatment for the new chart that you've created ("What is the reincarceration rate for Riders?")?

@hobuobi I've played with implementation and this treatment not working properly because of complex logic of the new chart and its highlighting. I think it takes some time to figure out how to make it work properly, so it's on you to decide.

nasaownsky · 2022-07-18T15:41:12Z

@macfarlandian Things are really strange, if I add clearAllMocks and resetAllMocks to afterEach, 5 tests passing inside of SupervisionSuccessRateMetric.test, without them 5 other tests passing, and rest of them not. I thought maybe it is something with timeout, since test messages says Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout, but adding jest.setTimeout not helping.

macfarlandian · 2022-07-18T17:15:31Z

@macfarlandian Things are really strange, if I add clearAllMocks and resetAllMocks to afterEach, 5 tests passing inside of SupervisionSuccessRateMetric.test, without them 5 other tests passing, and rest of them not. I thought maybe it is something with timeout, since test messages says Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout, but adding jest.setTimeout not helping.

the "timeout" error messages are confusing but common, they really just indicate that the test was waiting for a promise that never resolved (which would be one of the async functions called within ... it doesn't really tell you where it stalled unfortunately so if there is more than one await you just have to debug step by step to figure out how far it gets before timing out)

With intermittent failures like this the things I would probably investigate first include the test environment (including the global setup) and any known shared state across tests (e.g. demo data, the singleton RootStore). For me they fail even when running them individually so I actually don't really suspect a state leak across tests so much. Maybe it's more likely to be a problem with the test configuration? I spent a little more time poking at it this morning but honestly I still have no real leads on it

samanthahuff · 2022-07-18T20:05:19Z

Strange that the population over time includes decimals. @samanthahuff, sorry for not catching this earlier, but do you know why this might be the case?

The reason for the decimals is that this is the average daily population for each month. It looks like spotlight potentially counts total distinct people per month from what I can tell from the incarceration_population_by_prioritized_race_and_ethnicity_by_period view.

I originally modified a query that Kris sent me that was modeling the Key Sessions looker dashboard I believe, but I'm circling back with him about where that query came from. Would it be better to have total distinct people per month?

macfarlandian · 2022-07-18T20:12:32Z

It looks like spotlight potentially counts total distinct people per month from what I can tell from the incarceration_population_by_prioritized_race_and_ethnicity_by_period view.

This is correct, we use distinct person counts everywhere in Spotlight (either in point-in-time snapshots or aggregated over some time window). There's nothing to say we couldn't introduce average counts but you will definitely find that the UI always assumes these kinds of counts will be integers and person counts so we will probably at least want to do a thorough check that they're labelled correctly wherever they appear and that we're happy with how they're formatted

hobuobi

All of this looks good to me! Pending changes (to be handled separately):

Methodology for these sections
Copy for the final section
Small numbers treatment for the reincarceration plot
Other than that, should be good to go! Will await a technical team member's official signoff.

nasaownsky · 2022-07-19T15:47:49Z

@macfarlandian It seems like it crashes only because of demographic data section of SupervisionSuccessRateMetric.test (line 220). If disable it, the tests passes just fine.

samanthahuff · 2022-07-19T15:49:34Z

It looks like spotlight potentially counts total distinct people per month from what I can tell from the incarceration_population_by_prioritized_race_and_ethnicity_by_period view.

This is correct, we use distinct person counts everywhere in Spotlight (either in point-in-time snapshots or aggregated over some time window). There's nothing to say we couldn't introduce average counts but you will definitely find that the UI always assumes these kinds of counts will be integers and person counts so we will probably at least want to do a thorough check that they're labelled correctly wherever they appear and that we're happy with how they're formatted

Makes sense! I sent an updated csv to Humphrey with distinct totals. Code for that change can be found in the PR for the Rider analysis.

macfarlandian

@macfarlandian It seems like it crashes only because of demographic data section of SupervisionSuccessRateMetric.test (line 220). If disable it, the tests passes just fine.

Fascinating. I am still totally stumped by this, but since the metric itself seems to be working fine in the application and this is totally unrelated to your work I think you should just disable it and move on. Please leave a comment on the disabled tests that references this issue I just created for it: #589

I am doing one last review before approving, so just marking this as "request changes" to put a hold on it in the meantime! I am doing this now though so hopefully will be able to approve shortly.

macfarlandian

Noted a couple of very minor things I think you should address but nothing blocking. Great work!

macfarlandian · 2022-07-19T18:11:55Z

spotlight-client/src/VizCategoriesByDemographic/VizCategoriesByDemographic.tsx

+import React from "react";
+import CategoriesByDemographicMetric from "../contentModels/CategoriesByDemographicMetric";
+import DemographicFilterSelect from "../DemographicFilterSelect";
+import BarChartPair from "../RacialDisparitiesNarrativePage/BarChartPair";


because this component is not specific to the Racial Disparities narrative anymore I would move it out into its own folder (src/BarChartPair)

macfarlandian · 2022-07-19T18:23:04Z

spotlight-client/src/charts/BarChartCluster.tsx

+  justify-content: flex-end;
+`;
+
+type BarChartClucterProps = {


spotlight-client/src/charts/BarChartCluster.tsx

macfarlandian · 2022-07-19T18:37:10Z

spotlight-client/src/contentModels/SystemNarrative.ts

@@ -23,29 +23,31 @@ import {
 import Metric from "./Metric";
 import { MetricMapping, MetricRecord } from "./types";

+type NarrativeTypeId = SystemNarrativeTypeId;


this change doesn't really seem necessary

…into nasaownsky/518-riders-narrative

nasaownsky · 2022-07-21T11:22:52Z

@macfarlandian @hobuobi Should be ready to go now!

Base implementation of Riders narrative

41a5bf6

nasaownsky requested review from terryttsai, macfarlandian and hobuobi June 9, 2022 13:50

Added PopulationOverTime and Bubble sections of Riders narrative

040df7c

nasaownsky commented Jul 4, 2022

View reviewed changes

Added dual bars section

aa92142

Merge branch 'main' of https://github.com/Recidiviz/public-dashboard …

8da6214

…into nasaownsky/518-riders-narrative

nasaownsky added 2 commits July 11, 2022 21:04

Added Rates section & new BarChartCluster component

fd76fd7

lint fixes

1d43dbd

Added unknowns to HistoricalPopulationByCategoryMetric

b6f3bcf

Small improvements

f655338

hobuobi approved these changes Jul 19, 2022

View reviewed changes

macfarlandian mentioned this pull request Jul 19, 2022

Test failures in SupervisionSuccessRateMetric #589

Open

macfarlandian requested changes Jul 19, 2022

View reviewed changes

macfarlandian approved these changes Jul 19, 2022

View reviewed changes

nasaownsky added 5 commits July 20, 2022 17:42

Fixes

e918edb

Merge branch 'main' of https://github.com/Recidiviz/public-dashboard …

86453f3

…into nasaownsky/518-riders-narrative

Updated SupervisionSuccessRateMetric snapshot

678e734

Merge branch 'main' of https://github.com/Recidiviz/public-dashboard …

d7c742c

…into nasaownsky/518-riders-narrative

lint

6dcaf48

nasaownsky merged commit 06a64cd into main Jul 22, 2022

nasaownsky deleted the nasaownsky/518-riders-narrative branch July 22, 2022 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Riders narrative #576

Implement Riders narrative #576

nasaownsky commented Jun 9, 2022

coveralls commented Jun 9, 2022 •

edited

Loading

nasaownsky commented Jun 9, 2022

hobuobi commented Jun 10, 2022

macfarlandian commented Jun 10, 2022

nasaownsky commented Jul 4, 2022 •

edited

Loading

nasaownsky Jul 4, 2022

macfarlandian Jul 5, 2022

nasaownsky commented Jul 5, 2022 •

edited

Loading

hobuobi commented Jul 11, 2022

nasaownsky commented Jul 11, 2022

hobuobi commented Jul 12, 2022

macfarlandian commented Jul 12, 2022

nasaownsky commented Jul 13, 2022

nasaownsky commented Jul 18, 2022

nasaownsky commented Jul 18, 2022

macfarlandian commented Jul 18, 2022

samanthahuff commented Jul 18, 2022

macfarlandian commented Jul 18, 2022

hobuobi left a comment

nasaownsky commented Jul 19, 2022

samanthahuff commented Jul 19, 2022

macfarlandian left a comment

macfarlandian left a comment

macfarlandian Jul 19, 2022

macfarlandian Jul 19, 2022

macfarlandian Jul 19, 2022

nasaownsky commented Jul 21, 2022

Implement Riders narrative #576

Implement Riders narrative #576

Conversation

nasaownsky commented Jun 9, 2022

Description of the change

Type of change

Checklists

Development

Code review

coveralls commented Jun 9, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2711312681

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

nasaownsky commented Jun 9, 2022

hobuobi commented Jun 10, 2022

macfarlandian commented Jun 10, 2022

nasaownsky commented Jul 4, 2022 • edited Loading

nasaownsky Jul 4, 2022

Choose a reason for hiding this comment

macfarlandian Jul 5, 2022

Choose a reason for hiding this comment

nasaownsky commented Jul 5, 2022 • edited Loading

hobuobi commented Jul 11, 2022

nasaownsky commented Jul 11, 2022

hobuobi commented Jul 12, 2022

macfarlandian commented Jul 12, 2022

nasaownsky commented Jul 13, 2022

nasaownsky commented Jul 18, 2022

nasaownsky commented Jul 18, 2022

macfarlandian commented Jul 18, 2022

samanthahuff commented Jul 18, 2022

macfarlandian commented Jul 18, 2022

hobuobi left a comment

Choose a reason for hiding this comment

nasaownsky commented Jul 19, 2022

samanthahuff commented Jul 19, 2022

macfarlandian left a comment

Choose a reason for hiding this comment

macfarlandian left a comment

Choose a reason for hiding this comment

macfarlandian Jul 19, 2022

Choose a reason for hiding this comment

macfarlandian Jul 19, 2022

Choose a reason for hiding this comment

macfarlandian Jul 19, 2022

Choose a reason for hiding this comment

nasaownsky commented Jul 21, 2022

coveralls commented Jun 9, 2022 •

edited

Loading

nasaownsky commented Jul 4, 2022 •

edited

Loading

nasaownsky commented Jul 5, 2022 •

edited

Loading