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

Allow None in keys for partial dimension overlap #830

Merged
merged 2 commits into from Mar 6, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Aug 24, 2016

We allow dimensions with partial overlaps in their dimensions to be displayed together. The code will internally pad any dimension keys that need it with Nones so that the overall object has a consistent set of keys that it can be indexed by. However since NdMappings check that all dimension values must be in the list of Dimension.values the None padding will raise issues. This PR means that None is always allowed as a dimension value.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 31, 2016

Ready to merge, if you agree with the fix.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Sep 16, 2016

Although this PR can't be merged right now due to conflicts, I'm happy to discuss this proposal again now that None values have be enabled in DynamicMap keys for handling dimensioned stream values. As the plotting code has also been updated to handle dimensioned streams, I think it will be easier to see if we want to allow None values in this case...

@jlstevens jlstevens added this to the v1.7.0 milestone Oct 16, 2016

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 6, 2017

Let's merge this. It changes nothing but validation code so I don't think there's anything else to worry about.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 6, 2017

Ok, I think we should be ok with None values now. Merging.

@jlstevens jlstevens merged commit 277adc5 into master Mar 6, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+31.8%) to 78.25%
Details
s3-reference-data-cache Tests passing no test data changes required.
Details

@philippjfr philippjfr deleted the dynamic_partial_values branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment