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

Made sorting on NdMapping optional #1217

Merged
merged 6 commits into from Mar 24, 2017

Conversation

Projects
None yet
2 participants
@philippjfr
Copy link
Contributor

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 force-pushed the ndmapping_sort branch 2 times, most recently from 5d53e0d to 5159f37 Mar 17, 2017

@philippjfr philippjfr removed the WIP label Mar 24, 2017

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

@philippjfr

This comment has been minimized.

Copy link
Contributor Author

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

Contributor

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

Author Contributor

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

Contributor

Nice to see the simplified constructor!

This comment has been minimized.

@philippjfr

philippjfr Mar 24, 2017

Author Contributor

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

Contributor

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

Author Contributor

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

Contributor

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

Author Contributor

Will do.

This comment has been minimized.

@philippjfr

philippjfr Mar 24, 2017

Author Contributor

Added to the docstring above.

@jlstevens

This comment has been minimized.

Copy link
Contributor

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...

@philippjfr philippjfr force-pushed the ndmapping_sort branch from 9b5c0b4 to 2754854 Mar 24, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor

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
You can’t perform that action at this time.