-
Notifications
You must be signed in to change notification settings - Fork 18.9k
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
base: master
Are you sure you want to change the base?
Implement Smooth Sort #5236
Conversation
next step is to improve it to leonardo sort
Additionaly done the following to fix pervious issues:
|
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
from SortingAlgorithmTest
.
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 of Integer[]
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5236 +/- ##
============================================
+ Coverage 40.67% 41.97% +1.29%
- Complexity 2502 2602 +100
============================================
Files 519 522 +3
Lines 15447 15542 +95
Branches 2949 2968 +19
============================================
+ Hits 6283 6523 +240
+ Misses 8869 8725 -144
+ Partials 295 294 -1 ☔ View full report in Codecov by Sentry. |
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.
At the moment the code is way too complex. I would suggest to:
- implement
LeonardoHeap
(or use one of the existing implementations inheaps
), - use it in the implementation of
SmoothSort
.
|
||
int indexOfRightChild = rootNodeIndices.get(j) - 1; // right child is of level n-2 | ||
int indexOfLeftChild = rootNodeIndices.get(j) - 1 - getLeonardoNumbers()[currentLeonardoLevel - 2]; | ||
if (array[prevRootNodeIndex].compareTo(array[indexOfRightChild]) > 0 && array[prevRootNodeIndex].compareTo(array[indexOfLeftChild]) > 0) { |
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 to true
and array[prevRootNodeIndex].compareTo(array[indexOfLeftChild]) > 0
to false
.
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 trees are in decreasing order of levels
- Root nodes of the trees are in increasing order
- Each tree is in a 'max-heap' state.
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 imageAn 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.
leonardoTreeLevelforHeapify = currentLeonardoTreeLevels[j - 1]; | ||
} | ||
j = j - 1; | ||
if (j == i - 1) { |
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 never false
(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.
I did try to use existing implementaions of Heaps, however Leonardo Heap is a bit different from the others since this heap contains multiple loenardo trees of very specific shapes and the root node being the right most elemnent in the list. I can crate a new class |
… Branch_Smooth_Sort
Hi @vil02 , are any further changes required in the test cases? Or in the code perhaps? |
src/main/java/com/thealgorithms/datastructures/heaps/LeonardoHeap.java
Outdated
Show resolved
Hide resolved
src/main/java/com/thealgorithms/datastructures/heaps/LeonardoHeap.java
Outdated
Show resolved
Hide resolved
private static ArrayList<Integer> findConsecutiveLeonardoTreeIndices(int num) { | ||
int prevOneIndex = -1; | ||
int currentLevel; | ||
|
||
ArrayList<Integer> answer = new ArrayList<Integer>(); | ||
answer.add(-1); | ||
answer.add(-1); | ||
|
||
for (int i = 0; num > 0; i++) { | ||
currentLevel = num & 1; | ||
if (currentLevel == 1) { | ||
if (prevOneIndex != -1) { | ||
answer.set(0, prevOneIndex); | ||
answer.set(1, i); | ||
} | ||
prevOneIndex = i; | ||
} else { | ||
prevOneIndex = -1; | ||
} | ||
num >>>= 1; | ||
} | ||
return answer; | ||
} |
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.
This should be move to a separate class.
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 can create a new class for this, probably with the name of LeonardoTreeHelper.java
and add it's respective test file.
The other thing that can be done is that we can move this function to one of the bit manipulation classes? Since all this function is doing is returning first pair of indixes of consecutive 1 bits in an integer.
Which among these seems the right option @vil02?
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.
@JAPNITSINGH sorry, this was my mistake. I somehow thought that this computes the Leonardo lumbers. Please move these two methods back to the LeonardoHeap
. Sorry for the confusion.
Pushed some changes, I am yet to push some more. I will request review once done. |
Hi @vil02 I have made the changes listed below and needed a few inputs: Changes:
I wanted your inputs on this part: I have added a For context : |
private static ArrayList<Integer> findConsecutiveLeonardoTreeIndices(int num) { | ||
int prevOneIndex = -1; | ||
int currentLevel; | ||
|
||
ArrayList<Integer> answer = new ArrayList<Integer>(); | ||
answer.add(-1); | ||
answer.add(-1); | ||
|
||
for (int i = 0; num > 0; i++) { | ||
currentLevel = num & 1; | ||
if (currentLevel == 1) { | ||
if (prevOneIndex != -1) { | ||
answer.set(0, prevOneIndex); | ||
answer.set(1, i); | ||
} | ||
prevOneIndex = i; | ||
} else { | ||
prevOneIndex = -1; | ||
} | ||
num >>>= 1; | ||
} | ||
return answer; | ||
} |
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.
@JAPNITSINGH sorry, this was my mistake. I somehow thought that this computes the Leonardo lumbers. Please move these two methods back to the LeonardoHeap
. Sorry for the confusion.
int currentRootNodeIndex = rootNodeIndex; | ||
int rightChildIndex = rootNodeIndex - 1; | ||
int leftChildIndex = rootNodeIndex - LeonardoNumber.leonardoNumber(currentLevel - 2) - 1; | ||
int childIndexForSwap = -1; | ||
|
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.
int currentRootNodeIndex = rootNodeIndex; | |
int rightChildIndex = rootNodeIndex - 1; | |
int leftChildIndex = rootNodeIndex - LeonardoNumber.leonardoNumber(currentLevel - 2) - 1; | |
int childIndexForSwap = -1; | |
final int rightChildIndex = rootNodeIndex - 1; | |
final int leftChildIndex = rootNodeIndex - LeonardoNumber.leonardoNumber(currentLevel - 2) - 1; | |
final int childIndexForSwap = (heap.get(rightChildIndex).compareTo(heap.get(leftChildIndex)) >= 0) ? rightChildIndex : leftChildIndex; | |
final int currentRootNodeIndex = rootNodeIndex; |
Yes, the formatting in this repo is ugly.
} | ||
} | ||
|
||
private void shiftRootAndRestoreHeap() { |
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.
Do you see some change to chop this method into few smaller ones?
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.
Might be possible, I'll take a look into using some form of recursion instead of the while loop I am using. Recursion might give a bit more readability.
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 would suggest to refactor this method into a shorter one.
|
||
int rootNodeIndexForHeapify = rootNodeIndices.getLast(); | ||
int treeLevelforHeapify = currentTreeLevels[currentTreeLevels.length - 1]; | ||
boolean swaped = false; |
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.
boolean swaped = false; | |
boolean swapped = false; |
You can consider moving this declaration into the loop (smaller scope is always better). This should work, because this variable is always false
at the end of of each loop.
private void swap(int i, int j) { | ||
T temp = heap.get(i); | ||
heap.set(i, heap.get(j)); | ||
heap.set(j, temp); | ||
} |
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.
private void swap(int i, int j) { | |
T temp = heap.get(i); | |
heap.set(i, heap.get(j)); | |
heap.set(j, temp); | |
} | |
private void swap(final int i, final int j) { | |
Collections.swap(heap, i, j); | |
} |
make sure to
import java.util.Collections;
|
||
public T removeElement() { | ||
decreaseLevelTracker(); | ||
T element = heap.removeLast(); |
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.
T element = heap.removeLast(); | |
final T element = heap.removeLast(); |
Pushed some changes removed the Helper class and added |
Hey @vil02 , I have made some changes:
I tried to get rid of the |
Hi @vil02, would you kindly have a look and let me know if there are more changes to be made? |
src/main/java/com/thealgorithms/datastructures/heaps/LeonardoHeap.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
private void shiftRootAndRestoreHeap() { |
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 would suggest to refactor this method into a shorter one.
@Test | ||
public void testRemoveElement() { | ||
LeonardoHeap<Integer> heap = new LeonardoHeap<>(); | ||
heap.addElement(10); | ||
heap.addElement(20); | ||
heap.addElement(5); | ||
|
||
assertEquals(20, heap.removeElement()); | ||
assertEquals(10, heap.removeElement()); | ||
assertEquals(5, heap.removeElement()); | ||
} | ||
|
||
@Test | ||
public void testAddElementStrings() { | ||
LeonardoHeap<String> heap = new LeonardoHeap<>(); | ||
heap.addElement("z"); | ||
heap.addElement("a"); | ||
heap.addElement("x"); | ||
heap.addElement("b"); | ||
heap.addElement("y"); | ||
|
||
assertEquals("z", heap.removeElement()); // Max element should be z | ||
assertEquals("y", heap.removeElement()); | ||
assertEquals("x", heap.removeElement()); | ||
assertEquals("b", heap.removeElement()); | ||
assertEquals("a", heap.removeElement()); | ||
} | ||
|
||
@Test | ||
public void testRemoveElementString() { | ||
LeonardoHeap<String> heap = new LeonardoHeap<>(); | ||
heap.addElement("z"); | ||
heap.addElement("a"); | ||
heap.addElement("x"); | ||
|
||
assertEquals("z", heap.removeElement()); | ||
assertEquals("x", heap.removeElement()); | ||
assertEquals("a", heap.removeElement()); | ||
} | ||
|
||
@Test | ||
public void testAlwaysCurrentMaxElementIsRemoved() { | ||
LeonardoHeap<Integer> heap = new LeonardoHeap<>(); | ||
heap.addElement(5); | ||
heap.addElement(8); | ||
heap.addElement(7); | ||
heap.addElement(3); | ||
|
||
heap.addElement(4); | ||
heap.addElement(4); | ||
heap.addElement(4); | ||
heap.addElement(6); | ||
|
||
heap.addElement(8); | ||
heap.addElement(8); | ||
|
||
assertEquals(8, heap.removeElement()); | ||
assertEquals(8, heap.removeElement()); | ||
assertEquals(8, heap.removeElement()); | ||
assertEquals(7, heap.removeElement()); | ||
|
||
assertEquals(6, heap.removeElement()); | ||
assertEquals(5, heap.removeElement()); | ||
assertEquals(4, heap.removeElement()); | ||
assertEquals(4, heap.removeElement()); | ||
|
||
assertEquals(4, heap.removeElement()); | ||
assertEquals(3, heap.removeElement()); | ||
} | ||
|
||
@Test | ||
public void testForCompareChildAndSwap() { | ||
LeonardoHeap<Integer> heap = new LeonardoHeap<>(); | ||
Integer[] elements = {5, 33, 40, 28, 95, 29, 88, 94, 12, 84, 15, 33, 2, 52, 37, 62, 48, 13, 61, 59}; | ||
|
||
for (Integer element : elements) { | ||
heap.addElement(element); | ||
} | ||
|
||
assertEquals(95, heap.removeElement()); | ||
assertEquals(94, heap.removeElement()); | ||
assertEquals(88, heap.removeElement()); | ||
assertEquals(84, heap.removeElement()); | ||
assertEquals(62, heap.removeElement()); | ||
assertEquals(61, heap.removeElement()); | ||
assertEquals(59, heap.removeElement()); | ||
assertEquals(52, heap.removeElement()); | ||
assertEquals(48, heap.removeElement()); | ||
assertEquals(40, heap.removeElement()); | ||
assertEquals(37, heap.removeElement()); | ||
assertEquals(33, heap.removeElement()); | ||
assertEquals(33, heap.removeElement()); | ||
assertEquals(29, heap.removeElement()); | ||
assertEquals(28, heap.removeElement()); | ||
assertEquals(15, heap.removeElement()); | ||
assertEquals(13, heap.removeElement()); | ||
assertEquals(12, heap.removeElement()); | ||
assertEquals(5, heap.removeElement()); | ||
assertEquals(2, heap.removeElement()); | ||
} |
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 added some more assertEquals
here. A test, which does not assert can lead to confusion. Please have a look if it makes sense to parametrize these tests.
The LeonardoHeap
misses a method isEmpty()
. There is simply no way to know if user cans still removeElement
.
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.
@vil02 , I had a public heap size function earlier which was being used to check if the heap is empty (or heap size is 0). It was removed in one of the previous conversations. I can explicitly add an isEmpty() function instead of the heap size one.
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.
@JAPNITSINGH what can I say: it is my bad. Sorry for that. But honestly: I think having a isEmpty()
method makes sense.
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.
Alright, I'll make the changes in a couple of days and update here.
Hi @vil02 , I need a liitle more time to complete the tasks, I am a bit occupird with my college. I shall update here as soon as I get time to cmplete this. |
clang-format -i --style=file path/to/your/file.java