-
Notifications
You must be signed in to change notification settings - Fork 2
Circle of 5ths fixes and tests #25
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
Conversation
scale.lua
Outdated
| return self:rotate(4) | ||
| :add_alteration(4, -1) | ||
| :to_the_left_on_circle_of_5ths(iterations -1) | ||
| out = self:clone() |
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.
Sorry I don't get it.
return self:rotate(4)
:add_alteration(4, -1)
:to_the_left_on_circle_of_5ths(iterations -1)
is strictly equivalent to
out = self:clone()
for i = 1, iterations do
out = out:rotate(4):add_alteration(4, -1)
end
So we could probably keep the recursive implementation even with the fix.
What I don't get is
if out:count_alterations() < -7 then
out = out:to_the_right_on_circle_of_5ths(12)
end
I guess you need it somehow because it makes your tests pass but I don't know what it does and if it is that generic. For instance what happens if someone calls to_the_left_on_circle_of_5ths(123) ?
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.
What it does : If there are more than seven alterations on the scale (sharp of flat), there is an equivalent scale with only one of the other alteration (flat or sharp). To get it you have to rotate 12 on the opposite way.
If you want to allow double sharp or double flats, then the criterion would be out:count_alterations() < -14 . Would that would satisfy your need of generality ?
Although I agree that a recursive version is more elegant, it is also less efficient because the check would be done at each iteration. With a for loop, the check is done only at the end of the requested rotation.
to_the_left_on_circle_of_5ths(123) works with the proposed implementation. It will first add that many alterations, and then recursively call ten times to_the_right_on_circle_of_5ths(12). The result would be to_the_left_on_circle_of_5ths(3).
scale.lua
Outdated
| return self:rotate(5) | ||
| :add_alteration(7, 1) | ||
| :to_the_right_on_circle_of_5ths(iterations -1) | ||
| out = self:clone() |
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.
same comment here
No description provided.