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

Added next_permutation algorithm #405

Merged
merged 6 commits into from
Oct 10, 2021
Merged

Added next_permutation algorithm #405

merged 6 commits into from
Oct 10, 2021

Conversation

pratikgl
Copy link
Member

@pratikgl pratikgl commented Oct 9, 2021

Closes #404

Added next_permutation algorithm which rearranges a given array as a lexicographically greater permutation

#404 Halfway done

@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #405 (1f26916) into master (0edbada) will increase coverage by 0.017%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master      #405       +/-   ##
=============================================
+ Coverage   98.583%   98.601%   +0.017%     
=============================================
  Files           25        25               
  Lines         3319      3360       +41     
=============================================
+ Hits          3272      3313       +41     
  Misses          47        47               
Impacted Files Coverage Δ
pydatastructs/linear_data_structures/__init__.py 100.000% <ø> (ø)
pydatastructs/linear_data_structures/algorithms.py 99.715% <100.000%> (+0.037%) ⬆️

Impacted file tree graph

Comment on lines 1060 to 1062
output: Bool
Returns `True` if the function can rearrange the given array as a
lexicographically greater permutation, otherwise `False`.
Copy link
Member

@czgdp1807 czgdp1807 Oct 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the next permutation is obtained by modifying the input array i.e., instead of returning the permutation we are storing it in the input array itself. May be, we should provide a second option, in_place (by default it should be False). If set to True then array will be deep copied to a new one and that will contain the next permutation keeping the original input untouched.

Something like this,

def next_permutation(perm, in_place=False):
    if not in_place:
        array = ODA(int, len(perm))
        for i in range(len(perm)):
            array[i] = perm[i]
    else:
        array = perm
    ## Your algorithm
    return array

Note that in case, in_place is True then array should point to perm and hence, modifying array will modify the input perm as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A follow-up question:

So if in_place is True then instead of returning the boolean value we are returning the new permuted array (without mutating the original one) or we are returning both i.e boolean value and the permuted array.

Copy link
Member Author

@pratikgl pratikgl Oct 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can also remove this in_place thing altogether and do something like this return bool, array. If the user wants the in_place = False result they will do

# This way the original_array will be overwritten (in_place = False)
is_permute, original_array = next_permutation(original_array)

It will give users full control if they want to overwrite or not. However, as we will always be making a deep copy of the original array so even in the case of in_place = False it is consuming O(N) space

Copy link
Member

@czgdp1807 czgdp1807 Oct 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason behind returning a boolean? - I think it acts as a signal to the caller that the returned permutation is correct. It's a good to have return value.
From my understanding, in_place=True means overwrite the input array and in_place=False means do not overwrite the input array, create a new array for results instead. is_permute, original_array = next_permutation(original_array) is overwriting the original_array which I think is already happening in this PR. Always returning a new array is better IMO because next_permutation itself conveys something new will be returned (it is giving us the next permutation so we may expect that our input will remain the same). A function like sort conveys that they will sort the input array so modification of input is expected.

TLDR - Return both new array (do not modify the input array at all, create a copy and play with it) and boolean value. @pratikgl

@czgdp1807
Copy link
Member

czgdp1807 commented Oct 10, 2021

LGTM. Please add previous_permutation in a similar manner in this PR. @pratikgl

@czgdp1807
Copy link
Member

I did some refactoring and added tests. Once tests pass I will merge this PR.

@czgdp1807 czgdp1807 merged commit 0ed9ce1 into codezonediitj:master Oct 10, 2021
@czgdp1807
Copy link
Member

Thanks @pratikgl for this.

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

Successfully merging this pull request may close these issues.

Previous/Next Permutation
2 participants