-
Notifications
You must be signed in to change notification settings - Fork 12
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
ALV tree #24
Open
remshams
wants to merge
28
commits into
sashaafm:master
Choose a base branch
from
remshams:feat/AVL_Tree
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
ALV tree #24
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Adds comparable protocol for BST values * Adds standard implementation for BSTComparable * Updates insert functions to BSTStruct
* Standard method names for BST Comparable
* Used augmentation updates
* Does not perform rotations
* Separates methods for left- and right-heavy rotations
* Credo warnings
* Now preprocessing in delete since just the key of the node is provided. In case this is necessary in the future the delete method needs to be refactored to take a node as parameter.
* New method was added
* Cache for plt was removed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
AVL Tree
As discussed the separate PR for the AVL Tree implementation/suggestion. I've made some changes to BST tree implementation as well as added the AVL tree which "sits" on top of the BST.
Since the changes in the BST affect its general structure I'd love to to get some feedback.
The follwing gives a high level description about the changes. I think the details are best discussed at the specific code lines...
As mentioned please regard the current implementation as suggestion/
MVP
and basis for further improvements/extensionsGeneral
As already hinted at in the intro the AVL tree leverages the already existing BST (which in my opinion kind of makes sense since lots of operation can be reused).
For the AVL tree to work some changes were required in the BST:
BST Node
The bst nodes consist of:
The idea regaring the augmentation is/was somehow related to the general procedure for augmenting datastructures (as for example described in CLRS Chapter 14 3rd edition).
Comparable protocol
For the BST to work with any kind of data structure as value (e.g. structs as well as "simple" types a BST Comparable protocol was introduced that the value types stored in the BST are required to implement (as described in this google discussion there doesn't seem to be a "generic" comparable protocol which can be implemented).
The BST already contains an implementation/fallback for the
Any
type (falling back to Kernel).Hook methods
To avoid traversing the complete tree again for adding the AVL augmentations (
balance factor
andheight
) some kind of hooks were required to directly perform these calculations during BSTnew
,insert
anddelete
.These have been named
processors
and currently the BST supports a pre- and post processor.new
operation does not support that operation since the node does not yet exist)For the pre- as well as the postprocessor a standard identity processor is executed in case nothing is provided.
I made some thoughts whether to use a dedicated module or function keyword list. I decided for the latter as it somehow seems to be the elixir approach (but as I'm kind of new to elixir I could also be mistaken regaring that).
AVL Tree
The AVL tree implementation leverages the described BST extensions for performing the required operations.
Augmentation
It introduces an augmentation struct:
left.height - right.height
(however the ordering makes now difference)Postprocessor
A postprocess is added which performs the required rotations as well as node augmentations.
After every rotation the augmentation values are "updated".
The summarized steps are:
Although in the case of insert it would be sufficient to just check the parant of the newly inserted node, the immutable character of elixir/erlang required all nodes on the path from the root to the affected node to be traversed.
Tests
Tests for the BST updates as well as the AVL tree operations have been added.
The test dedicated to the AVL tree should cover the different rotation cases (please let me know in case I've missed cases).
Other
I had some trouble to get the dialyzer going through. It seems that it has issues with structs as well as protocols. Therefore a
dialyzer.ignore-warnings
file has been added to get a green build again ;). I'd love to hear if someone knows how to avoid these warnings.I've found a elixir talk regarding that issue and it looks like this is somehow a bug. However the talk is from 2014 so maybe there have been some changes regarding that.
Also worth mentioning is that I do not get the issue on my local machine, it just appears during the travis build.
The displayed credo issue is just a warning regarding the
@spec
definition for the greater- and less implementations for theComparable
protocol.In case I have missed anything else please let me know ;)