Feature/create breaks #25
Conversation
src/executeQuery.js
Outdated
| const _ = require('lodash') | ||
|
|
||
| function breaksQuery (features, query, options) { | ||
| // TODO: add check if query is valid (or should this have been handled in query?) |
There was a problem hiding this comment.
what types of validation are you interested in?
There was a problem hiding this comment.
I don't think we need this. We also may not need the check on the next line either.
src/executeQuery.js
Outdated
| function breaksQuery (features, query, options) { | ||
| // TODO: add check if query is valid (or should this have been handled in query?) | ||
| const queriedData = standardQuery(features, query, options) | ||
| if (queriedData === undefined || queriedData.features === undefined) throw new Error('query resposne undefined') |
src/executeQuery.js
Outdated
| if (queriedData.features.length === 0) throw new Error('need features in order to classify') | ||
|
|
||
| try { | ||
| const classification = options.classificationDef |
There was a problem hiding this comment.
I'm not a fan of wrapping large chunks of code in try catch, what's your reasoning here?
There was a problem hiding this comment.
Understandable, but I couldn't find any catches in winnow. Throwing an error causes the tests to break. Alternative?
src/executeQuery.js
Outdated
| return calculateClassBreaks(queriedData.features, classification) | ||
| } else if (classification.type === 'uniqueValueDef') { | ||
| const { options, query } = calculateUniqueValue(queriedData.features, classification) | ||
| let thing = aggregateQuery(queriedData.features, query, options) |
There was a problem hiding this comment.
no need to define this variable
There was a problem hiding this comment.
testing artifact, already handled
src/generateBreaks/normalization.js
Outdated
| const normField = classification.normalizationField | ||
| if (normField) { | ||
| const normValues = getFieldValues(features, normField) | ||
| if (Array.isArray(normValues)) { |
There was a problem hiding this comment.
this function is tough to read, should probably be broken into a few smaller ones
test/aggregate.js
Outdated
| t.equal(Array.isArray(results[0]), true) | ||
| t.equal(results.length, 5) | ||
| t.deepEqual(results[0], [0, 2.8]) | ||
| t.deepEqual(results[4], [11.3, 14]) |
There was a problem hiding this comment.
I think its good to have at least one test that validates all of the individual breaks
There was a problem hiding this comment.
already done, just wasn't pushed
test/aggregate.js
Outdated
| t.plan(1) | ||
| const options = _.cloneDeep(classBreaks) | ||
| options.classificationDef.classificationMethod = 'invalidMethod' | ||
| const results = winnow.query(features, options) |
There was a problem hiding this comment.
this should throw an error
test/aggregate.js
Outdated
| t.equal(Array.isArray(results), true) | ||
| t.equal(typeof results === 'object', true) | ||
| t.equal(results.length, 156) | ||
| t.deepEqual(results[0], { Genus: 'MAGNOLIA', count: 3778 }) |
There was a problem hiding this comment.
what does the full response look like here?
There was a problem hiding this comment.
An array of objects with the aggregation value and field aggregated on.
[ { count: 3778, Genus: 'MAGNOLIA' }, { count: 7164, Genus: 'QUERCUS' }, { count: 84, Genus: 'XYLOSMA' }, ...]
| "classificationMethod": "esriClassifyEqualInterval", | ||
| "breakCount": 5 | ||
| }, | ||
| "where": "Trunk_Diameter<15", |
There was a problem hiding this comment.
would be good to do a test without a where clause and/or with where=1=1
There was a problem hiding this comment.
works when deleting where clause, but not where=1=1...
| @@ -0,0 +1,28 @@ | |||
| { | |||
| "classificationDef": { | |||
There was a problem hiding this comment.
it's good that we're supporting the geoservices version of a classification definition. we also want to support a simplified version within winnow. basically the pattern is to shorten the key names and smooth over any weird stuff from the geoservices api
There was a problem hiding this comment.
is this so that winnow isn't dependent on an input format from FS?
README.md
Outdated
| ##### `Class Breaks` | ||
| _Class Breaks_ is used to classify numeric data based on a number of breaks and a statistical method. Features can also be normalized before being classified. | ||
|
|
||
| ```json |
We will likely need to add more tests. Close to finishing uniqueValue breaks, but not present in this PR.