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

Made sorting on NdMapping optional #1217

Merged
merged 6 commits into from Mar 24, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Member

philippjfr commented Mar 17, 2017

Let's users provide an explicitly sorted NdMapping. Still need to add a warning when a regular dict and sort=False are supplied since sort order is nondeterministic in that case.

Edit: Widgets are currently assuming sorted keys and NdElement seems to have been sorting in some cases.

Addresses: #1216

Fixes: #1042

@philippjfr philippjfr added feature WIP and removed WIP labels Mar 17, 2017

@philippjfr philippjfr requested a review from jlstevens Mar 24, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 24, 2017

Ready to review/merge when tests pass.

@@ -49,11 +49,11 @@ def __init__(self, enabled):
self.enabled = enabled
def __enter__(self):

This comment has been minimized.

@jlstevens

jlstevens Mar 24, 2017

Member

The docstring of this context_manager should be updated. As now sort=False is valid, it should just say it disables sorting regardless of whether the NdMapping has sort=True or sort=False.

I also think the line 'Should only be used if values are guaranteed to be sorted before or after the operation is performed.' should just say something else - maybe just that the initial ordering (whatever it is) should be preserved?

This comment has been minimized.

@philippjfr

philippjfr Mar 24, 2017

Member

Docstring updated.

if name in own_params}, **params)
if isinstance(initial_items, MultiDimensionalMapping):
params = dict(util.get_param_values(initial_items),
**dict({'sort': self.sort}, **params))

This comment has been minimized.

@jlstevens

jlstevens Mar 24, 2017

Member

Nice to see the simplified constructor!

This comment has been minimized.

@philippjfr

philippjfr Mar 24, 2017

Member

That's where the bug reported in #1042 was, this was written very early on before we had utilities for this kind of thing.

if not self._instantiated and self.get_dimension(dim).values == 'initial':
if val not in vals:
self._cached_index_values[dim.name].append(val)
elif vals and val is not None and val not in vals:

This comment has been minimized.

@jlstevens

jlstevens Mar 24, 2017

Member

Was this chunk of logic made redundant by some other change or was it always redundant?

This comment has been minimized.

@philippjfr

philippjfr Mar 24, 2017

Member

This was my early hacky approach to define a fixed order, I think it has been broken for a long time.

with item_check(False):
return util.ndmapping_groupby(self, dimensions, container_type,
group_type, sort=sort, **kwargs)
group_type, sort=True, **kwargs)

This comment has been minimized.

@jlstevens

jlstevens Mar 24, 2017

Member

Probably worth mentioning either in a comment - or maybe even the docstring - that sorting will always be applied.

This comment has been minimized.

@philippjfr

philippjfr Mar 24, 2017

Member

Will do.

This comment has been minimized.

@philippjfr

philippjfr Mar 24, 2017

Member

Added to the docstring above.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 24, 2017

Looks good. Most of my comments relate to documentation issues...

I agree a ValueError is better than a warning when sort=False and an unsortable dictionary is supplied. It is probably best to completely disallow the nondeterministic case rather than simply warning.

Edit: The tests do seem to be failing though...

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 24, 2017

Tests are now fixed (pr build is passing at least) and you've made the docstring changes I suggested. Merging!

@jlstevens jlstevens merged commit 8cc5160 into master Mar 24, 2017

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.5%) to 76.944%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the ndmapping_sort branch Apr 11, 2017

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