Made sorting on NdMapping optional #1217
Conversation
philippjfr
force-pushed
the
ndmapping_sort
branch
2 times, most recently
from
5d53e0d
to
5159f37
Compare
|
Ready to review/merge when tests pass. |
| @@ -49,11 +49,11 @@ def __init__(self, enabled): | |||
| self.enabled = enabled | |||
|
|
|||
| def __enter__(self): | |||
There was a problem hiding this comment.
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?
| if name in own_params}, **params) | ||
| if isinstance(initial_items, MultiDimensionalMapping): | ||
| params = dict(util.get_param_values(initial_items), | ||
| **dict({'sort': self.sort}, **params)) |
There was a problem hiding this comment.
Nice to see the simplified constructor!
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Was this chunk of logic made redundant by some other change or was it always redundant?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Probably worth mentioning either in a comment - or maybe even the docstring - that sorting will always be applied.
There was a problem hiding this comment.
Added to the docstring above.
|
Looks good. Most of my comments relate to documentation issues... I agree a Edit: The tests do seem to be failing though... |
philippjfr
force-pushed
the
ndmapping_sort
branch
from
9b5c0b4
to
2754854
Compare
|
Tests are now fixed (pr build is passing at least) and you've made the docstring changes I suggested. Merging! |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
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