Isolines and data files separated by time resolution #62
Conversation
src/core/chart.js
Outdated
|
|
||
| //Remove variable_name; it's redundant with variable_id (and takes | ||
| //up a lot of room.) | ||
| variation.splice(variation.indexOf("variable_name"), 1); |
There was a problem hiding this comment.
There needs to be a check that variable_name is actually in variation before attempting to remove it. Unlike unique_id, variable_name is not always present.
|
I'll go through this in a bit more detail, but one honest question is: how much of this is a "rewrite" vs. "new functionality". I think that you've billed it as a rewrite, but I'm suspicious of adding 1300 LOCs without the balance of some code removal. Are there things where you have completely replaced their use and could be removed? Or is there now psudo-duplicated functionality that may confuse future programmers on this code base? Again, I'm asking out of ignorance having not delved into the codebase (yet), so it's an honest question. |
|
Good question! It's intended to replace (There are also some functional differences; |
|
Oh, wait, you meant the whole branch, not just At this point I'd just like to have more eyes on chart.js, which rewrites some of the C3 graph generation functions to 1) work with split chronologies, and 2) hopefully be easier to understand, to make sure I'm on the right track and it all makes sense before I finish updating the rest of the graph generation code to work with split chronologies (and hopefully be easier to understand). Sorry for being unclear! |
There was a problem hiding this comment.
I have a few comments, but overall your approach looks reasonable. I might request a review from @rod-glover as well.
src/components/AppMixin/AppMixin.js
Outdated
|
|
||
| //Temporary kludge until we decide whether the multimeta API | ||
| //call should return climatology period data. | ||
| //FIXME: get rid of this. |
There was a problem hiding this comment.
I don't have an inherent problem with FIXME's for the future, but it's sometimes helpful to future readers to specific under what conditions should be in place at the time of fixing. I.e. you could replace "until we decide whether" with something like: "this is necessary until climatology period data is available through the API".
src/components/AppMixin/AppMixin.js
Outdated
|
|
||
| this.setState({ | ||
| meta: models, | ||
| model_id: models[0].model_id, | ||
| variable_id: models[0].variable_id, | ||
| experiment: models[0].experiment, | ||
| }); | ||
| //this.bodgeClimatologyPeriodIntoMetadata(); |
src/components/AppMixin/AppMixin.js
Outdated
| //call should return climatology period data. | ||
| //FIXME: get rid of this. | ||
| models = _.map(models, function(model){ | ||
| var params = model.unique_id.split('_'); |
There was a problem hiding this comment.
Yeah, we really don't want to be relying on string munging of unique_id, so hopefully this "temporary kludge" will be very short lived.
| @@ -43,12 +45,41 @@ var DataController = React.createClass({ | |||
| this.setTimeSeriesNoDataMessage("Loading Data"); | |||
| this.setClimoSeriesNoDataMessage("Loading Data"); | |||
| this.setStatsTableNoDataMessage("Loading Data"); | |||
|
|
|||
There was a problem hiding this comment.
Seems like you could DRY this up. Each of these functions is a _.find() call matching model_id, variable_id and experiment + some number of other constraints. Can _.find() calls be chained?
src/core/chart.js
Outdated
| * and return a C3 graph object displaying them. | ||
| * | ||
| * It takes an arbitrary number of data objects, but no more than | ||
| * two separate unit types. Allowable data resolutions are monthly(12), |
There was a problem hiding this comment.
Does it allow both timeseries and climatologies? By the 12/4/1, I would assume only climatologies, but it would be good to clarify.
| //Variables with different units require seperate axes. | ||
| //C3 can theoretically support indefinite numbers of y-axes, | ||
| //but that would be hard for a user to make sense of, | ||
| //so it's capped at two here. |
| c3Data.axes[timeseriesName] = "y2"; | ||
| } | ||
| else { | ||
| throw new Error("Error: too many data axes required for graph"); |
There was a problem hiding this comment.
Reasonable to throw and error here and not assume responsibility. Are you making sure that we catch it somewhere at a higher level?
There was a problem hiding this comment.
Agree. JavaScript is very good at sailing invisibly over errors, which, uncaught, appear only in the console.
There was a problem hiding this comment.
Also, nitpick: No need to repeat the word "Error" in the string in the Error object.
There was a problem hiding this comment.
Errors thrown here are caught by DataController and displayed to the user via DataController.displayError(). They appear on the graph instead of data. That is, the graph displays "Loading" while the API is queried and the graph is generated, and then either data or an error message is displayed.
I like having "Error:" in the error messages that will be displayed on the graph in place of data because I feel it makes it clearer to the user that their data isn't going to show up eventually if they wait, but I'm not too attached to the idea.
There was a problem hiding this comment.
OK, good.
Aside: I might consider factoring out the "Error" into DataController.displayError(), but that's a minor matter and probably not worth your time at this point.
src/core/chart.js
Outdated
|
|
||
| /* | ||
| * Helper function for parseTimeseriesForC3. | ||
| * Accepts a dataseries object with 1, 4, or 12 timestamp:value pairs |
There was a problem hiding this comment.
Hmmm, so the idea is that monthly is the lowest common denominator for all of the supported data types (monthly/seasonal/annual climatologies) and you'll downscale everything that's coarser to monthly? That seems like an OK strategy for 1/4/12/17 resolution, but I definitely would not want to do this for higher frequency timeseries (e.g. a 150 year annual timeseries). Keep those use cases in mind for the future.
There was a problem hiding this comment.
My thought exactly. Please add a comment to that effect here.
| //TODO: special case climatological period to display as (XXXX-XXXX) | ||
| //TODO: possibly cue descriptors to appear in a specific order? | ||
| // "Tasmin Monthly Mean" sounds better than "Monthly Tasmin Mean". | ||
| var shortestUniqueTimeseriesNamingFunction = function (metadata, data) { |
There was a problem hiding this comment.
This is an interesting function, and I think that it's a really nice way to separate out some funky heuristics that will really improve usability and readability of the charts. It does have a lot of weird cases though, so if you don't have tests in place yet, I'd advise that you write some.
There was a problem hiding this comment.
Yes, this is a very nice bit of functional programming.
I do also support writing tests. Heuristics especially are tricky, and the tests document what you mean to do.
src/core/chart.js
Outdated
| /* | ||
| * Simple formatting function for numbers intended for display. | ||
| */ | ||
| //TODO: look up significance algorithms and write a smarter one! |
There was a problem hiding this comment.
This is how we do it in pacificclimate/pdp: https://github.com/pacificclimate/pdp/blob/master/pdp/static/js/pdp_controls.js#L215
There was a problem hiding this comment.
Thanks! That's really helpful.
| * seasonal (4), or yearly (1); an error will be thrown | ||
| * if this function is called on data with another time resolution. | ||
| */ | ||
| var timeseriesToAnnualCycleGraph = function(metadata, ...data) { |
There was a problem hiding this comment.
Just a little aside: Is there some reason we prefer this var f = function(args) ... style declaration to the simpler function f(args) ... style?
| } | ||
|
|
||
| if(timestamps.length != expectedTimestamps[timescale]) { | ||
| throw new Error("Error: inconsistent time resolution in data"); |
There was a problem hiding this comment.
There are two ways this error condition can be fulfilled:
- timescale is valid, but the length doesn't match the expected length derived from timescale.
- timescale isn't in the valid set (so
expectedTimestamps[timescale] === undefined
It would be helpful to include the values of both timestamps.length and timescale in the message. Also, optionally, the expected resolutions, which you can obtain from Object.keys(expectedTimestamps).
Better still perhaps would be to split this check into two parts:
- Derive expected length (and error when timescale isn't valid).
- Check
timestamps.length
There was a problem hiding this comment.
Also, what catches these errors and what is done about it? Sorry if the answer ought to be obvious.
There was a problem hiding this comment.
DataController and its siblings (DualDataController and MotiDataController) catch these errors. The only thing that's done with them is that their text is displayed to the user, on the graph, in place of the expected data visualization.
There was a problem hiding this comment.
Right, I knew that ... :)
| } | ||
|
|
||
| //Seasonal timeseries need one month of winter removed from the beginning of the | ||
| //year and added at the end, since winter wraps around the calendar new year. |
|
This branch still isn't completely ready for merging (or at least review), but it's getting close. The functionality is pretty much there, but some cleanup and testing are needed. Outstanding task list:
Cleanup
Tests
|
|
Working on the logscale bug discovered during the demo this morning. Here are two calls to ncWMS: They are completely identical except the |
|
Aha! Figured it out. NcWMS won't generate maps with logscaled colouration for any data that contains negative values. Which makes some sense, though the result being an xml parse error is confusing. So during the demo, we were only able to view precipitation with logscaled colour ranges, as precipitation is never negative. Temperature, which includes negative data, cannot be coloured logarithmically by ncWMS, and the map disappeared when we tried. So now my question is: what should I do about it? Options, from easiest to hardest:
Comments? Any bright ideas I'm overlooking? |
|
From a programming perspective, I like something between 2 and 3. 2 won't necessarily work 100% of the time with climate data because a) log scaled won't work with data == 0 (which precip often is) and b) some models may show negative precip I think. I'd like something such as: default to using minmax but whitelist a few variable names to enable log? Keep in mind that you can set WMS params to limit the range of the plot (I.e. Set a minimum to trace for log requests). |
|
Huh, neat. What would negative precip mean in the context of a model that outputs it? |
|
Since this branch is already 3k-ish lines, I've decided logscale functionality and associated complexity belongs on another branch. I took the logscale UI elements off the MapController dialogue and incorporated Rod's excellent suggestions. |
| updateSelection: function (param, selection) { | ||
| var update = {}; update[param] = selection; | ||
| this.setState(update); | ||
| }, | ||
|
|
||
| //this function updates the timestamp displayed on the map and triggers | ||
| //a rerender. |
There was a problem hiding this comment.
Technically, this code only updates the time values in the state. The consequences of that are to update the map display and trigger a renderer. In fact, the consequences of that update could change (if other code changes), presumably, so better to state this as 'nominally ...' or 'typically ...' or omit entirely unless this consequence is critical to understanding something here.
| this.requestTimeMetadata(uniqueId).then(response => { | ||
| this.selectedDataset.times = response.data[uniqueId].times; | ||
| //this function stores a dataset selected by the user to state, | ||
| //triggering a rerender to show the new dataset. |
There was a problem hiding this comment.
Similar comment to just above. It's a bit clearer here, but documenting the likely effects of an update in a pub-sub system is perilous, and establishes, in comments, a coupling between parts that pub-sub by design decouples.
| } | ||
| //select an arbitrary index to display initially. It will be a | ||
| //0th index, but could be January, winter, or Annual. | ||
| var startingIndex = Object.keys(times)[0]; |
There was a problem hiding this comment.
Danger! A hash is unordered. Hash keys are not guaranteed to be returned in the order they are stored. So the value initially displayed could be any of the values stored.
If that's OK behaviour, this code is OK. If not (and it seems a bit weird to allow an effectively random initial value), you need to add some kind of order-awareness -- perhaps the simplest would be to store the first index as it is generated.
| else { | ||
| sPalette = nextProps.comparandMeta ? 'seq-Greys' : 'x-Occam'; | ||
| cPalette = nextProps.comparandMeta ? 'x-Occam': undefined; | ||
| } |
There was a problem hiding this comment.
Hmm, a trifle tricky to understand. I'd go with a single if-else statement here instead of the two conditional assignments.
| cPalette = nextProps.comparandMeta ? 'x-Occam': undefined; | ||
| } | ||
|
|
||
| //generate a list of timestamps available for the specified data |
|
|
||
| //select a starting timestamp to display. | ||
| //should correspond to a 0th index, but might be annual, seasonal, or monthly | ||
| var startingTime = Object.keys(times)[0]; |
There was a problem hiding this comment.
... to here is AFAICT identical to most of updateDataset above.
Factor that out into a shared function? If you don't, please apply the comment there about unordered hash keys here too.
| //a user could decide to view. | ||
| //Not sure JSON is the right way to do this, though. | ||
| //TODO: see if there's a more elegant way to handle the callback | ||
| //(selector won't pass an object) |
There was a problem hiding this comment.
Agree could be more elegant.
The obvious alternative (but not thought through here in detail) would be to use an index into a list, implicit -- you'd need to establish a canonical ordering for that --, or explicit -- you'd need to share a data structure for that.
| if (ids.length > 1) { | ||
| datasetSelector = (<Selector | ||
| label={"Select Dataset"} | ||
| onChange={this.updateDataset} | ||
| items={ids} value={this.state.dataset} | ||
| items={ids} value={`${this.state.run} ${this.state.start_date}-${this.state.end_date}`} | ||
| value={selectedDataset} |
There was a problem hiding this comment.
Why is value specified twice here? And what does that mean?
There was a problem hiding this comment.
Whoops, I think it means I wasn't paying enough attention. :)
| value={this.state.contourPalette} | ||
| /> | ||
| ); | ||
| //TODO: |
src/components/Map/CanadaMap.js
Outdated
| map.addControl(new NcWMSColorbarControl(this.ncwmsContourLayer, { | ||
| position: 'bottomright' | ||
| })); | ||
| } |
There was a problem hiding this comment.
I think the following might be easier to read and less repetitive:
if(this.props.scalarDataset) {
scalarAs = new NcWMSAutoscaleControl(this.ncwmsScalarLayer, {
position: 'bottomright'
});
scalarCb = new NcWMSColorbarControl(this.ncwmsScalarLayer, {
position: 'bottomright'
});
}
if(this.props.contourDataset) {
contourAs = new NcWMSAutoscaleControl(this.ncwmsContourLayer, {
position: 'bottomright'
});
contourCb = new NcWMSColorbarControl(this.ncwmsContourLayer, {
position: 'bottomright'
});
}
if(this.props.contourDataset && this.props.scalarDataset) {
map.addControl(scalarCb);
scalarAs.addLayer(this.ncwmsContourLayer);
map.addControl(scalarAs);
map.addControl(contourCb);
}
else if(this.props.contourDataset) {
map.addControl(contourAs);
map.addControl(contourCb);
}
else if(this.props.scalarDataset) {
map.addControl(scalarAs);
map.addControl(scalarCb);
}
What do you think?
There was a problem hiding this comment.
Or better yet:
function makeControls(layer) {
return [
new NcWMSAutoscaleControl(layer, {
position: 'bottomright'
}),
new NcWMSColorbarControl(layer, {
position: 'bottomright'
})
]
}
if(this.props.scalarDataset) {
[scalarAs, scalarCb] = makeControls(this.ncwmsScalarLayer);
}
if(this.props.contourDataset) {
[contourAs, contourCb] = makeControls(this.ncwmsContourLayer);
}
...
|
Reviewed MapController.js and CanadaMap.js. A few suggestions, otherwise looking very good. |
There was a problem hiding this comment.
Hrm, I had a bunch more (than 2) comments that seem to have evaporated from the GH page. Grrr. This looks like a great piece of work to me. I had a few small nitpick comments, but they're not worth holding up the merge (and all of the dependent work in the queue). Thanks for taking this on!
| var timeseriesPromises = []; | ||
|
|
||
| //fetch data from the API for each time resolution that has a dataset. | ||
| //the "monthly" time resolution is guarenteed to exist, but |
There was a problem hiding this comment.
Why is monthly guaranteed to exist? I don't think that's a safe assumption (e.g. annual ClimDEX datasets).
| * DataControllerMixin.js - shared functionality for data controllers | ||
| * | ||
| * This mixin is added to MotiDataController, DataController, and | ||
| * DualDataController. Those three controller components have different |
There was a problem hiding this comment.
Good explanation of its purpose!
| }else { | ||
| // either an error thrown by a data validation function, | ||
| // an error thrown by the DataGraph or DataTable parsers, | ||
| // or the generic and somewhat unhelpful "Network Error" from axios |
There was a problem hiding this comment.
Does axios give you the HTTP status code? If you can differentiate between a 500 error (error.response == 500) that would probably be useful for the client to know...
There was a problem hiding this comment.
Usually axios provides an HTTP status code in error.response. Once in a while it doesn't, and provides only the "Network Error" message without populating error.response or error.request. We are not the only ones who have encountered intermittent lack of http status on axios errors.
As far as I could tell from testing, the lack-of-http-status-code in our case happened when the backend's status was 500, but not every time. Perhaps it was related to how long the backend took to return a response.
This may be fixed, I see axios' most recent update changelog (0.16.2) includes
- Including underlying request in errors
| * returns metadata for another dataset that: | ||
| * - matches all attribute/value pairs in the "difference object" | ||
| * - matches the original dataset for any attributes not in "difference" | ||
| * (Unique_id is ignore for purposes of matching datasets.) |
There was a problem hiding this comment.
nitpick: s/ignore/ignored/
| * would return the annual-resolution dataset that corresponds to a monthly one. | ||
| * Returns only one dataset's metadata even if multiple qualify. | ||
| */ | ||
| findMatchingMetadata: function(example, difference, meta = this.props.meta) { |
| * | ||
| * Child of (and viewer for) MapController. | ||
| * | ||
| * Props passed to configure shaded color map: |
There was a problem hiding this comment.
This isn't a comment on the code necessarily, but why are you naming all of the raster layer with the prefix "scalar". "Scalar" to me implies one single number, and in my mind (and the language we use), we've never called that map a scalar map...
There was a problem hiding this comment.
ncWMS2 calls the shaded colour map style "scalar," so I was using "scalar" to distinguish that ncWMS raster layer from the other "contour" styled (isoline) ncWMS raster layer. I'm not sure why ncWMS calls that layer style "scalar".
Suggestions for more intuitive-to-us names for the two ncWMS layers? Colours & isolines? Raster & isolines? Chloropeth and isolines?
There was a problem hiding this comment.
Huh, well if ncWMS calls it that, then I guess that's more sensible. I'd generally vote for raster and isolines, but if you have already used the language of ncWMS, then perhaps we should align with it.
| }).then(response => { | ||
| this.layer.setParams({ colorscalerange: response.data.min + ',' + response.data.max }); | ||
| }); | ||
| for(var i = 0; i < this.layers.length; i++) { |
There was a problem hiding this comment.
Kind of a nitpick, but why can't you do this.layers.forEach() here as well?
|
@rod-glover You had your requested suggestions addressed, correct? |
|
@corviday can we review these changes side-by-side? I'm having trouble determining what changes you made in response to my comments. This is by no means a criticism, it's just that this is a huge and long-running PR and it's hard to figure out. |
|
In general I think this is a very good piece of work. |
EDIT: this branch is now ready for a merge-oriented review.
This isn't ready to merge yet, but I'd appreciate some feedback on
chart.jsspecifically at this juncture. Chart.js containstimeSeriesToAnnualCycleGraph()and its helper functions, which create an annual cycle graph from multiple timeseries. The impetus for this rewrite was the fact that monthly, yearly, and seasonal data are now separate series, but it should produce a reasonable graph from arbitrary timeseries combinations. (For example, comparing two variables on the isoline view.)I'm particularly interested in readability/clarity, since between the arbitrary C3 object format requirements and the arbitrary 17-point chronology requirements, it's been tough to write clear graphing code on this project. I've tried to be clear about what's going on and why, and would like to hear about anywhere it isn't.