Fix Histogram Binning by juliawu · Pull Request #2086 · datacommonsorg/website

juliawu · 2023-01-23T21:34:48Z

Makes the following changes to the histogram tile:

Adds support for binning when dates include days (YYYY-MM-DD)
Don't show histogram if there's no data to show

Before:

After:

Before:

After:

…to hist-fix

beets

thanks for this! now that we have a bit of a breather, we can make this more robust.

beets · 2023-01-24T19:20:20Z

+      data_found = False
+
+      # Get the latest month we have data for
+      latest_date = series[-1].get('date', '')


check for the length of series first (it could be empty)

good point! This section got removed in the revamp, but will keep in mind for the future.

beets · 2023-01-24T19:21:31Z

+      latest_month = 12
+      if latest_date[:4] == year and len(latest_date) >= 7:
+        latest_month = int(latest_date[5:7])


nit: try to avoid "magic numbers", even if it's for something like date parsing.

Thanks for the pointer, removed magic numbers.

beets · 2023-01-24T19:24:23Z

+      months_to_fill = [
+          f"{year}-{str(mm).zfill(2)}" for mm in range(1, latest_month + 1)
+      ]


let's get this working for cross-year boundaries (which will kick in next week :))

beets · 2023-01-24T19:26:48Z

+  if (!histogramData || _.isEmpty(histogramData.dataPoints)) {
    return null;
  }
  const rs: ReplacementStrings = {


perhaps only add the ChartTileContainer when there is data to plot (otherwise the empty frame with no chart is a little awkward).

added this check in the new version as well.

chejennifer

the updated histogram looks good!

chejennifer · 2023-01-24T21:50:41Z


  useEffect(() => {
-    if (histogramData) {
+    if (histogramData && !_.isEmpty(histogramData.dataPoints)) {


nit: maybe make a function shouldShowHistogram() & use it both here and in line 77

…to hist-fix

juliawu · 2023-02-09T00:39:21Z

Following offline conversation, the histogram tile has now been revamped:

Histogram tile will now be used in the disaster event type block
Data binning now happens on client-side, not server-side
Data presented uses the same data fetch as the other tiles on the block
Data presented updates dynamically when users update selection in the date dropdown.

Further changes that will come in future PRs are:

Special logic for binning when the user selects "last 30 days"
Support for binning by day/week instead of just month

Sample screenshots of the new histogram tile:

chejennifer

nice, thanks for the update!

chejennifer · 2023-02-09T15:41:29Z

+          replacementStrings={rs}
+          className={"histogram-chart"}
+          allowEmbed={false}
+          getDataCsv={() => dataPointsToCsv(histogramData)}


nit: since allowEmbed is set to false, setting this getDataCsv field is unnecessary

Thanks for the pointer, removed getDataCsv.

chejennifer · 2023-02-09T15:45:16Z

-      processData(props, rawData, setHistogramData);
-    }
-  }, [props, rawData]);
+  // specify months if dateSetting is just a year


nit: looks like we're specifying dates, not months?

Good catch, updated this comment line to clarify we're specifying dates

chejennifer · 2023-02-09T15:48:28Z

-    }
-  }, [props, rawData]);
+  // specify months if dateSetting is just a year
+  if (dateSetting.length == "YYYY".length) {


this is assuming that if dateSetting length is YYYY, then getDateRange will return a date range that is also YYYY. That is true right now, but I think it would be better to check the lengths of startDate and endDate instead.

Good idea, switched to checking lengths of startDate and endDate

chejennifer · 2023-02-09T15:50:41Z

+  // imputing 0s for dates the source(s) may not yet have data for.
+  if (!_.isEmpty(disasterEventPoints)) {
+    const lastDate =
+      disasterEventPoints[disasterEventPoints.length - 1].startDate;


are you taking a sorted list of disasterEventPoints? If so, please mention it in the function comments that disasterEventPoints should be sorted by date

Added to function comments!

but it doesn't look like you're sorting the data before passing it into this function?

Apologies, I misinterpreted your original comment. I've added sorting inside the function.

chejennifer · 2023-02-09T15:58:41Z

+  // Fill in months between start and end dates
+  const months = new Array<string>();
+  for (
+    let date = new Date(startDate);


nit: because new Date() creates a date in local time and .toIsoString() gets the ISO string of the UTC date, we would want to create a UTC date here by doing new Date(startDate + "Z") like this: https://github.com/datacommonsorg/website/blob/master/static/js/utils/disaster_event_map_utils.tsx#L164-L168

Thanks for the pointer! Added + "Z".

chejennifer · 2023-02-09T16:02:38Z

-    props.statVarSpec[0].unit
-  );
+function shouldShowHistogram(histogramData: DataPoint[]): boolean {
+  return histogramData && !_.isEmpty(histogramData);


nit: _.isEmpty will check for nulls and undefined too, so really only need to keep the ~_.isEmpty check here

thanks, kept only the !_.isEmpty. Leaving the function despite its simplicity because "shouldShowHistogram" is more readable to me.

…to hist-fix

beets

thanks for the updates!

beets · 2023-02-09T22:16:54Z

-      #   stat_var_key: "fire_count"
-      # }
+      tiles {
+        # TODO: Need to specify P1Y


you can delete this comment if it doesn't apply anymore (ditto for the rest)

beets · 2023-02-09T22:17:33Z

 }

+message HistogramTileSpec {
+ string event_type_key = 1;


super nit: please check indent spacing

Good eye! Fixed spacing

beets · 2023-02-09T22:32:59Z

+              disasterEventData[tile.histogramTileSpec.eventTypeKey] || {
+                eventPoints: [],
+                provenanceInfo: {},


we should be handling these default values before this (i.e. disasterEventData should have reasonable defaults for all event types). or the component should handle the case where disasterEventData is null. of these, i might choose (2)

Went with (2). Added handling so that histogram just doesn't render if disasterEventData is null instead of breaking the entire component.

beets · 2023-02-09T22:36:31Z

-    }
-  }, [props, rawData]);
+  // specify full dates if start and end dates are just YYYY
+  if (startDate.length == "YYYY".length && endDate.length == "YYYY".length) {


what if the range is specified YYYY-MM?

Added YYYY-MM case handling.

beets · 2023-02-09T22:53:13Z

+    const eventMonth = event.startDate.slice(0, "YYYY-MM".length);
+
+    // Increment count in corresponding bin
+    if (bins.has(eventMonth)) {


perhaps add a log warning if we're skipping events.

added a log message if we skip events

beets · 2023-02-09T22:53:32Z

+
+  for (const event of disasterEventPoints) {
+    // Get start time in YYYY-MM
+    const eventMonth = event.startDate.slice(0, "YYYY-MM".length);


check that all events have at least YYYY-MM granularity

added check

beets · 2023-02-09T22:54:45Z

+function shouldShowHistogram(histogramData: DataPoint[]): boolean {
+  return !_.isEmpty(histogramData);


this is ok to keep, but for future reference, might be too little to have as a helper.

thanks for the pointer! noted.

chejennifer

thanks for the updates!

…to hist-fix

This is a followup PR to #2086 which makes the histograms use the color of the event types instead of default red. Because this is a followup, diffs currently include all changes made in #2086. I'll message out when #2086 is merged and the diffs are easier to read. Screenshots: <img width="1024" alt="Screenshot 2023-02-09 at 10 02 21 AM" src="https://user-images.githubusercontent.com/4034366/217902251-e962cddb-43df-429d-a15f-3df9d092ab65.png"> <img width="1026" alt="Screenshot 2023-02-09 at 10 02 40 AM" src="https://user-images.githubusercontent.com/4034366/217902271-71e6ac0e-f95f-4da6-8e09-3f857e96a1cb.png"> <img width="1025" alt="Screenshot 2023-02-09 at 10 02 58 AM" src="https://user-images.githubusercontent.com/4034366/217902288-b08c2cd7-93db-43b7-9342-b82f3409d9d5.png"> <img width="1021" alt="Screenshot 2023-02-09 at 10 03 23 AM" src="https://user-images.githubusercontent.com/4034366/217902317-fe4207b9-b789-40a3-95d2-696e025e3126.png"> <img width="1023" alt="Screenshot 2023-02-09 at 10 03 12 AM" src="https://user-images.githubusercontent.com/4034366/217903076-d5c1d6de-a6a7-49a2-9c53-a0c94ed1754e.png"> <img width="1029" alt="Screenshot 2023-02-09 at 10 03 34 AM" src="https://user-images.githubusercontent.com/4034366/217902337-0928c6ca-6815-4f18-990d-f5ff1dc64af0.png"> <img width="1026" alt="Screenshot 2023-02-09 at 10 03 53 AM" src="https://user-images.githubusercontent.com/4034366/217902366-861d6daa-d218-42fc-aa1d-6bf21bf2fb67.png">

@chayim

Fix binning and remove empty charts

a3112bc

juliawu requested review from beets and chejennifer January 23, 2023 21:34

juliawu and others added 4 commits January 23, 2023 13:36

Merge branch 'master' into hist-fix

3fcf21d

Merge branch 'master' of https://github.com/datacommonsorg/website in…

50e32f1

…to hist-fix

lint

a35074e

remove debugging statements

d03cdf9

beets reviewed Jan 24, 2023

View reviewed changes

chejennifer reviewed Jan 24, 2023

View reviewed changes

juliawu added 14 commits January 27, 2023 09:42

Merge branch 'master' of https://github.com/datacommonsorg/website in…

6e47972

…to hist-fix

remove magic numbers

441fe20

Merge branch 'master' of https://github.com/datacommonsorg/website in…

69a5989

…to hist-fix

Merge branch 'master' of https://github.com/datacommonsorg/website in…

c69d13a

…to hist-fix

response to reviewer comments pt 1

de8dbb6

move histogram to disaster event block

79e56b2

fix histogram chart titles

483db7f

cleanup props

e4a9c10

lint

acb6bd5

Merge branch 'master' of https://github.com/datacommonsorg/website in…

cdf3ec5

…to hist-fix

fix series.py merge errors

8a9088b

update allowed tile types test

a295bb3

fix key error and lint

60610e5

fix drought histogram title

2dfef78

juliawu requested review from beets and chejennifer February 9, 2023 00:40

chejennifer reviewed Feb 9, 2023

View reviewed changes

juliawu added 3 commits February 9, 2023 08:37

Merge branch 'master' of https://github.com/datacommonsorg/website in…

5d41646

…to hist-fix

respond to reviewer comments

27622ab

remove unneccessary ChartTileContainer prop

5883cb8

remove unused import

3749c99

juliawu mentioned this pull request Feb 9, 2023

[Disaster Dashboard] Update histogram colors #2221

Merged

sort disasterEventPoints in getMonthsArrray()

73fa537

beets reviewed Feb 9, 2023

View reviewed changes

juliawu added 2 commits February 9, 2023 15:05

remove old 'need to specify P1Y' comments

7ec4df8

fix spacing issue in subject_page.proto

b4f4cd9

chejennifer approved these changes Feb 10, 2023

View reviewed changes

juliawu added 4 commits February 9, 2023 18:51

add YYYY-MM handling in getMonthsArray

9fee1f8

handle null case

01166ed

add check for monthly granularity and console log statement

3788d06

Merge branch 'master' of https://github.com/datacommonsorg/website in…

92062b4

…to hist-fix

juliawu merged commit a5f89b1 into datacommonsorg:master Feb 10, 2023

juliawu mentioned this pull request Feb 13, 2023

Histogram display issues #1942

Closed

juliawu deleted the hist-fix branch May 25, 2023 16:51

		function shouldShowHistogram(histogramData: DataPoint[]): boolean {
		return !_.isEmpty(histogramData);

Uh oh!

Conversation

juliawu commented Jan 23, 2023

Uh oh!

beets left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chejennifer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliawu commented Feb 9, 2023

Uh oh!

chejennifer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beets left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!