Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement Smooth Sort #5236
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
Implement Smooth Sort #5236
Changes from 16 commits
9e5cb32
9ff4e85
69b24a8
6de4059
d6a14b6
384f814
29252e1
4f0fd70
0cdcba2
d32e435
712868e
368db8a
5f9d2e7
990b183
0eda98e
aed3358
ab905bf
65ca159
d5e3b95
ec02a6c
b19bd38
5d21701
951653f
79a5d20
9fe137c
7aa1421
645b29c
89d9f51
1adbd15
c249579
edbf08a
cf1ce78
369cbeb
01ba460
fa0797f
ba730d0
a8a5835
5093f86
761b848
37c794b
c0b8737
ad4c540
0edabcd
6b4c415
174de3f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see there is no test with
array[prevRootNodeIndex].compareTo(array[indexOfRightChild]) > 0
evaluating totrue
andarray[prevRootNodeIndex].compareTo(array[indexOfLeftChild]) > 0
tofalse
.To ensure that the logic is correct, please add a missing test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I shall add a specific test case in
SmoothSortTest.java
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why this is happening and I shall try my best to explain in this comment.
Leonardo Heap has the following three properties that define it (Going through the array from left to right):
The Level-1 leonardo Tree(L1) and Level 0 leonardo Tree (L0) are the 'base case' trees as they have exactly one node.
At any given valid leonardo heap, if the heap contains both L1 and L0 tree then the element in L0 tree will always be greater than element at L1 tree, because of the second property mentioned above ( L0 >= L1 )
When L2 tree is being constructed, the level0 tree becomes the right child level1 tree becomes the left child. And hence it is not possible to have a condition such that level0 < prevRootNodeVal < level1 as it would be a voilation of the above.
This section of the wiki page and this article (under the image
An erroneous swap
) mention that root needs to be bigger than the new element and the roots of its child nodes for a swap to happen. However this article(in page 6) mentions that the preRootValue should exceede its largest son.I believe the statement in wiki is more genreic statement than a technical one and this condition can be optimized to just having
array[prevRootNodeIndex].compareTo(array[indexOfLeftChild]) > 0
. I shall however look around for more articles that suggest otherwise before I make changes to this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
j == i - 1
is neverfalse
(among the existing test cases).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do have a test case (not currently committed) with around 900 entries where this evalueates to false during creation of the heap. I can try to reduce the entires in such a way that the condition still evaluates to true, I will for sure end up having an array of size 20-30 ( as i will need 3 trees of non consecutive levels .... L1 L3 and L5 at least ).
Would it be fine if I added a test case in
SmoothSortTest.java
with around 20-30 elements in the test? If yes, I shall work on it at the very end after resolving the other conversations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Yes, this condition is never false. I belive what I intented to do was run
maxHeapifyLeonardoTree
if you reached the left most tree and further swaps are still present.I am checking this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the answer to the above questions is yes, then if should be enough to derive the
SmoothSortTest
fromSortingAlgorithmTest
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is possible, I just have to use generic
T[]
in place ofInteger[]
modify the code a little bit and use.compareTo()
and it should be good. I shall work on it and update this PR. I'll comment here once changes are ready.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the code, haven't pushed it yet. The smooth sort algorithm I implemented seems to be failing for most arrays of size greater than 700. I guess the problem is with how I am calculating the left child of a node. I am having a look into it. I shall provide an update here once I am done with the fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated to code, derived the class from SortAlgorithm, removed unnecessary return statements , renamed variables to make the code more understandable.