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

plug timeline into pipeline #39

Merged
merged 26 commits into from
Nov 1, 2021
Merged

Conversation

cnell-usgs
Copy link
Member

@cnell-usgs cnell-usgs commented Oct 3, 2021

This takes the chart structure implemented in and connects it to the R pipeline so that the timeline is defined by the data (#27).

Pipeline changes @lindsayplatt :

  • Adding public/gw-conditions-time-labels.csv target to define labels on the timeline. Right now this is just month and year, but could also be used to add annotations to the timeline based on any date/date range and paired annotation.
  • This depends on new target gw_time which is the full date range of the GW data with variable day_seq to order the animation.
  • gw_time is used to organize the site-level timeseries data (gw_anomaly_data_w_colors). Before we had a variable wyday sequencing the animation that was defined on a site basis, which was causing dates to shift and be out of sync for sites with missing data.
  • modified 3_visualize/src/build_peaks_svg.R to just write the svg for the basemap, as the peaks are all drawn with D3. This might require deeper thinking re: performance, thoughts here: Peaks map - optimize SVG #18.

These new targets need be added to AWS to read them in.

Vue/js changes @hcorson-dosch :

  • breaks up the timeline drawing function into pieces and structures callback function
  • add function to set scales globally; rescope unused global variables
  • defining color scale in single place, added 2 alternative scale options (evaluate color scale #38)
  • uses public/gw-conditions-time-labels.csv to add labels to timeline x-axis
  • adds a y-axis to the time chart.
  • re-organizes grid
  • addresses misalignment of dates & chart position

New look:
image

time_df <- tibble(Date = seq.Date(from = date_start,
to = date_end,
by = "1 day")) %>%
mutate(day_seq = as.numeric(rownames(.))) %>%
Copy link
Contributor

Choose a reason for hiding this comment

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

dplyr::row_number() will do the same thing as as.numeric(rownames(.))

2_process/src/prep_data_for_visualizing.R Outdated Show resolved Hide resolved
@@ -26,3 +29,28 @@ add_colors_to_data <- function(data_in, scico_palette_nm = "roma") {
quant_category == "Very low",
yes = col_palette[5], no = "black"))))))
}
generate_time <- function(data_in) {
date_start <- min(gw_anomaly_data$Date)
date_end <- max(gw_anomaly_data$Date)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding na.rm = TRUE just in case there is an NA date.

2_process/src/prep_data_for_visualizing.R Outdated Show resolved Hide resolved
ungroup() %>%
date_full <- data_in %>%
# create a row for every date x site
mutate(site_no = as.character(site_no)) %>%
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary? The site_no column should already be character. If not, we should fix here where it is loaded rather than deal with it downstream.

Could change this to:

gw_anomaly_data:
    command: read_csv("2_process/out/gw_data_anomalies.csv", col_types = I('cDnnc'))


gw_time %>%
left_join(date_full) %>%
left_join(data_in %>% mutate(site_no = as.character(site_no))) %>%
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing the step above to change how this data is read in would simplify this line to left_join(data_in)

2_process/src/prep_data_for_visualizing.R Outdated Show resolved Hide resolved
expand(Date, site_no) %>%
distinct()

gw_time %>%
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a comment about what is happening here and why would be very useful! After running the code and exploring, I think that these left joins effectively filter the data so that each site has values for the first day of every month in the data set.

command: build_peaks_svg(
target_name,
data_in = gw_anomaly_data_w_colors,
sites_sf = gw_sites_sf,
Copy link
Contributor

Choose a reason for hiding this comment

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

You took out these two arguments but the function itself, still has these included in its definition.

for(s in sites) {
site_coords_svg <- sites_sf %>%
filter(site_no == s) %>%
convert_coords_to_svg(svg_width = svg_width, view_bbox = st_bbox(generate_usa_map_data()))
Copy link
Contributor

Choose a reason for hiding this comment

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

So you are no longer using the SVG with the sites already included? You are adding via D3?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I left some notes about that here #18 (comment)

I think it's worth returning to this to draw the initial map from the pipeline, and then grab those paths with D3 for the animation. I was hitting some blocks with how it was set up though due to missing sites when there wasn't data on the first day, and the way I was modifying them with D3 (was creating duplicate elements). This PR addresses those issues but is slower to load.


this.line = this.d3.line()
.defined(d => !isNaN(d))
.x((d, i) => this.xScale(this.days[i]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

After going back and reviewing how percData is built, I see that the perc values are the same length as the # of day_seq in this.days, which is why this works. But I wonder if it would work to instead pass this.line all of d in line 358 where you are adding the line chart, and then within this line function, pass d.day_seq to the xScale and d.perc to the yScale, so that the x and y values come from the same dataset. But I can't remember if d3.line() will accept a full dataset, or if it prefers a single array, in which case this approach works fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is what I want to do but cannot get it to work. It SHOULD work I think I'm having an indexing issue

@@ -460,8 +569,9 @@ export default {
.attr("transform", "translate(90, 0)")

var legend_keys = ["Very low", "Low", "Normal", "High","Very high"]; // labels
var legend_color = [this.verylow, this.low, this.normal,this.normal, this.high, this.veryhigh];
var shape_dist = [5,25,42,42,60,83,103]; // y positioning (normal has 2 shapes butted together)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this legend is great, but the vertical alignment between the labels and the shapes seems like it might vary by browser (or maybe window size), which is a pain. Not sure it's worth fiddling with too much, but for reference here's what I see in edge (left; monitor fullscreen) vs in firefox (right; monitor ~2/3 of full screen):

image

Copy link
Collaborator

@hcorson-dosch-usgs hcorson-dosch-usgs Oct 26, 2021

Choose a reason for hiding this comment

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

One other note on the legend - when I first saw the page, I had to check the line chart title to confirm that GWL stood for groundwater level. I realize that would be long to spell out here, so not sure what the solution is, but I'm not sure that people who see the page will know what GWL means, and I think the eye hits that legend before the line chart, since the legend is closer to the upper left.

Copy link
Member Author

Choose a reason for hiding this comment

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

New version:
image

I like that the glyphs show the movement of the peaks, but what I don't like is that we are binning the data into 5 uneven groups and not showing that in some way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also cleaned this up so Normal is a single glyph and can be coded in a much cleaner way

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, that's a good point. Could we incorporate a histogram somehow, to show the distribution? I also like the idea of labeling the breakpoints, rather than the individual ranges, so there's less repeated text.

Copy link
Member Author

Choose a reason for hiding this comment

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

How would you do that here with the breakpoints?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a blurry ugly rough sketch, but I think you could label the breakpoints like so (w/ or w/o the histogram):
IMG-4641

.text(function(d){ return d})
.attr("text-anchor", "start")
.style("alignment-baseline", "middle")

},
drawFrame1(data){
drawFrame1(map_svg, data){
// draw the first frame of the animation
const self = this;

// select existing paths using class
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this comment outdated? It looks like anomaly_peaks.svg no longer has any existing paths of class gwl_glpyh, and that you're creating the paths from scratch here, with d3, but maybe I'm looking in the wrong place?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to address this in the next PR. Short answer, yes, but I'm going to add back in.

// draw axes
var xliney = svg.append("g")
.call(xLine)
.attr("transform", "translate(20," + 120 + ")")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just add comments to note what these nudges are doing? In case anyone needs to adjust things in the future ?

// control animation
this.animateLine(this.start);
this.animateGWL(this.start);
this.playButton(map_svg, this.width*(2.5/7) , 300);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better to place the play button off the map - maybe lower left?

Copy link
Member Author

@cnell-usgs cnell-usgs Nov 1, 2021

Choose a reason for hiding this comment

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

Moved to own container on the lefthand side and implemented suggested changes (contributing to #40)
image

.text(function(d, i) { return d.name })
.attr("text-align", "middle")
.on('click', function(d, i) {
.text(function(d, i) { return d.month_label })
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this text gets too large if fullscreen on a monitor:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this if you start with a smaller screen size and then make it larger? On the initial draw, all font sizes should be consistent, BUT they scale in the bottom chart when the window is changed. I documented this here (#34) and am not sure how to address it. Would you have a change in window size trigger a function that redraws just that chart?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh good catch - yes it was. As for when the window is changed, I'm not sure what the best way is to go about it. I think in the past I've either used a fixed font size (instead of em), or set media breaks and defined different font sizes. I don't think either is ideal, though.

}
// annotated timeline
.liney {
stroke-width: 2px;
color: rgb(143, 206, 131);
}

// desktop
Copy link
Collaborator

@hcorson-dosch-usgs hcorson-dosch-usgs Oct 29, 2021

Choose a reason for hiding this comment

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

Shoot looks like on a laptop part of the map and the entire line chart are hidden:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also - for what it's worth, the non-desktop option looks great on a tall narrow window on a monitor, since it makes the map larger (but may not be worth having so many grid versions):
image

Copy link
Member Author

Choose a reason for hiding this comment

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

I designed the layout mobile first, which I think helped. Ideally everything is in view at once regardless of screen size. I've adjusted the height parameter for the desktop version so it fills the space in the wide version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ugh I wish editing these grids for different windows wasn't so fiddly! The chart is now drawing on my laptop screen, so that's great. Though now on my full monitor view, you have to scroll down to see the chart which isn't ideal:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's how it looks with recent changes on laptop:
image
I adjusted for relative font scaling and set a max-height on desktop using vh


button
.on("mousedown", function() {
self.animateLine(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So to get this working, replace lines 427 and 428 with

self.pressButton(self.isPlaying)

so that it reads:

button
    .on("mousedown", function() {
        self.pressButton(self.isPlaying)
    });


// trigger animation if animation is not already playing
if (playing == false) {
self.animateChart_Map()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then replace this line with

self.animateLine(0);
self.animateGWL(0);

so that it reads

pressButton(playing) {
      const self = this;

      // trigger animation if animation is not already playing
      if (playing == false) {
            self.animateLine(0);
            self.animateGWL(0);
      }
},


// undim button
let button_rect = this.d3.selectAll(".play_button").selectAll("rect")
.style("fill", 'rgb(250,109,49)')
Copy link
Collaborator

Choose a reason for hiding this comment

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

And change this color to "steelblue"

.range([mar, line_width-mar])

// set indicator for play button
self.isPlaying = true

Copy link
Collaborator

Choose a reason for hiding this comment

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

And add this chunk to dim the play button:

// dim play button rectangle
let button_rect = this.d3.selectAll(".play_button").selectAll("rect")
button_rect
    .style("fill", "#d6d6d6")

} else {
this.d3.selectAll(".hilite")
.transition()
.duration(this.day_length)
.attr("x", x(this.days[this.n_days]))
.attr("x", self.xScale(this.days[this.n_days-1]))
}
Copy link
Collaborator

@hcorson-dosch-usgs hcorson-dosch-usgs Oct 29, 2021

Choose a reason for hiding this comment

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

Then, after this line, add the following section to reset the play button and reset isPlaying to 'false' (might need to tweak that delay slightly -- seems a bit long for some reason though it seems like that should be right):

// once animation has completed, reset color of play button
// and set isPlaying to false
button_rect
    .transition()
    .delay(this.day_length*(this.n_days-1))
    .on("end", self.resetPlayButton);

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks this was helpful.

Copy link
Collaborator

@hcorson-dosch-usgs hcorson-dosch-usgs left a comment

Choose a reason for hiding this comment

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

This is so cool, Cee! Love the animated line chart. I've left a number of comments -- not sure if you want to address them before merging or later -- may be a mix of both?

@cnell-usgs
Copy link
Member Author

Merging. Initial draw edits to come in next PR.

@cnell-usgs cnell-usgs merged commit bb03cd2 into DOI-USGS:main Nov 1, 2021
@cnell-usgs cnell-usgs deleted the time-chart branch December 28, 2021 18:32
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.

3 participants