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

Control sort from .overlay/.layout #4088

Open
wants to merge 5 commits into
base: master
from

Conversation

@DancingQuanta
Copy link
Contributor

DancingQuanta commented Nov 6, 2019

Currently, the elements of a kdim subjected to .overlay or .layout are sorted by default.

This PR is to explore how to let a user disable sorting through these methods.
The proposed API is

obj.overlay('kdim', sort=False')
@DancingQuanta DancingQuanta changed the title Control sort from `.overlay`/`.layout` Control sort from .overlay/.layout Nov 6, 2019
@DancingQuanta DancingQuanta force-pushed the DancingQuanta:sort branch 2 times, most recently from 598532e to a1b2f73 Nov 8, 2019
@DancingQuanta

This comment has been minimized.

Copy link
Contributor Author

DancingQuanta commented Nov 8, 2019

I think I have identified three issues here and dealt with them.

  • Test test_idxmapping_groupby_unsorted used incorrect data to test against where a test list is sorted instead of unsorted
  • ndmapping_groupby.groupby_pandas does not pass sort to group_type, container_type or pd.DataFrame.groupby. In all three cases, sort=True by default.
  • MultiDimensional.groupby passes sort=True to ndmapping_groupby which does not make any use of sort passed to gropuby.

I tried to resolve above issues and made sure to proprogate sort to newer MultiDimensional objects. I wanted to add sort to get_param_values however this conflicts with sort in function arguments and I have no idea how to prioritize one over other.

@DancingQuanta DancingQuanta force-pushed the DancingQuanta:sort branch from b095cae to efb984c Nov 8, 2019
@DancingQuanta

This comment has been minimized.

Copy link
Contributor Author

DancingQuanta commented Nov 9, 2019

Is groupby_python still needed? The test suite include pandas dependency and so this function groupby_python never get tested.

@DancingQuanta

This comment has been minimized.

Copy link
Contributor Author

DancingQuanta commented Nov 9, 2019

The overlay and layout methods are now able to pass sort to the groupby after these changes in the PR.
@philippjfr Can you review ? This PR changed more than 2 years old code and so i am not sure if I have introduced any behaviour changes beyond what I am aiming for with overlay and layout.

@DancingQuanta DancingQuanta force-pushed the DancingQuanta:sort branch from efb984c to 2138e36 Nov 9, 2019
@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Nov 14, 2019

The functionality seems fine to me!

At a glance, the code also seems fine and I don't think there are backwards compatibility issues as sort should just override the behavior. That said, @philippjfr might spot some issues that I haven't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.