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

Fix/join datasets #411

Merged
merged 5 commits into from
Mar 28, 2018
Merged

Fix/join datasets #411

merged 5 commits into from
Mar 28, 2018

Conversation

tomwayson
Copy link
Member

@tomwayson tomwayson commented Mar 27, 2018

this is a WIP, everything is working, just need to iron out a few wrinkles and squash

resolves #406

add dataset.ts and joinDatasets()

using flattenData and joinData instead of flattenFeatureSets

move getChartData to datasets.js as getData() for testing

also make it the only export

clarify tests
@tomwayson
Copy link
Member Author

This is why Jest is dumb

it's still dumb to run tests for browser code in node
@tomwayson tomwayson changed the title DNM Fix/join datasets Fix/join datasets Mar 28, 2018
@benstoltz
Copy link
Member

So visually everything looks okay with a multi-series dataset, however if there was a missing value it doesn't fill in with a 0, it instead fills in with a blank value. The result looks something like this:

screen shot 2018-03-28 at 8 32 21 am

Now if it was a record of 0 then it would look like (bear in mind the middle value has no val assigned to it):
screen shot 2018-03-28 at 8 32 33 am

I suppose the question is, do we want this type of behavior, or do we want it filling in a record of 0?

@tomwayson
Copy link
Member Author

Glad you brought that up. I meant to include screenshots and explain the same. I actually had the code to add those, but the only way I could think to do that requires another loop through each value field for each dataset for each row, so I thought the above behavior was sufficient, and it wasn't worth the overhead

This morning I had the thought that we could add an option like fillInMissing, but where, passed to show()? On the chart definition itself, like under dataset? Either way, that's something I think we can add later as a feature, but I do think it's worth taking another look at the API I've made and in this PR and thinking about how we would pass that and/or other options. For example, we could have an option to force calling getAttributes() on every row instead of just checking the first one for the extremely rare situation where you have mixed features and non-features.

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.

Multi-series charts mis-chart values when attribute missing from query response
2 participants