Scale identification

[CNelias]

This file allows one to identify which scale is played in an array of notes. I will need to modify it for the case where there is not a single scale played, like in most jazz standards (but this is more complicated, especially when the players starts playing outside the key).

[Datseris]

Maybe you want to clean up the code a bit before I do a proper review? I see a lot of things that are commented, etc. Ping me (using @Datseris) when this is ready for review and the test files are in this pull request.

[Datseris]

Please also abide by this: https://docs.julialang.org/en/v1/manual/style-guide/index.html#Use-naming-conventions-consistent-with-Julia-base/-1

[CNelias]

@Datseris The code is cleaner now and I've included the test, but I'm not sure that it has the correct syntax (it has to load an external file to test).

[CNelias]

Why does it need to be declared as const ? This just throws an error when trying to fill the Dict with it's values (because it's like trying to redefine a constant variable). Also Dict{String,Vector{String}} even though it's more elegant seems to mess the code up.

[CNelias]

Ths problem with using pitch_to_name is that it also gives the octave (C0, D6, F4 etc...) and using

MIDI_to_notes(notes) = [pitch_to_name(n.pitch)[1:end-1] for n in notes]

sometimes conserves the octave indication when the note is not sharp or flat. one obtains a list like ["F#","G6","G#","A6","A#"] and so on, which is not what i want here. it would be possible to write an if condition taking care of that, but in the end it would take more time than using my MIDI_to_notes function.

[Datseris]

First of all, please start replying to my comments instead of the main review page. It isn't harder, it doesn't cost you anything, you just click reply at a different position in this webpage. Yet it makes the review process so much easier.

Why does it need to be declared as const ? This just throws an error when trying to fill the Dict with it's values (because it's like trying to redefine a constant variable). Also Dict{String,Vector{String}} even though it's more elegant seems to mess the code up.

It has to be a const because it is declared in global scope and used within a functions. You should read the Julia manual on performance tips: https://docs.julialang.org/en/v1/manual/performance-tips/index.html

Being a const has nothing to do with change its values because it is a mutable object. You are clearly doing something wrong. Here is an example:

julia> const d = Dict{String, Vector{String}}()
Dict{String,Array{String,1}} with 0 entries

julia> d["a"] = ["a", "b"]
2-element Array{String,1}:
 "a"
 "b"

it would be possible to write an if condition taking care of that, but in the end it would take more time than using my MIDI_to_notes function.

This is not an argument. It obviously takes more time to write good code. And good code re-uses existing good code, which is also well tested. Your MIDI_to_notes isn't tested anywhere. Besides, such a trivial change takes literally 10 seconds. Here is the solution, using well-test and simple basic code:

julia> notes = getnotes(midi, 4)
533 Notes with tpq=960
 Note F4  | vel = 69  | pos = 7427, dur = 181
 Note A♯4 | vel = 85  | pos = 7760, dur = 450
 Note D5  | vel = 91  | pos = 8319, dur = 356
  ⋮
 Note G4  | vel = 109 | pos = 190748, dur = 590

julia> pitch_to_fundamental(notes) = [MIDI.PITCH_TO_NAME[mod(n.pitch, 12)] for n in notes]
pitch_to_fundamental (generic function with 1 method)

julia> pitch_to_fundamental(notes)
533-element Array{String,1}:
 "F"
 "A♯"
 "D"
 "D"
 "G♯"
 "A♯"
 "G"
 "A♯"
 "F♯"
 "C"
 "G"
 "F"

Your MIDI_to_notes function is just bad Julia code by the way. It initializes numerous lists without any reason and it also initializes them as Any, which is unreasonable.

---

Finally, as you can clearly see yourself, the automated tests are failing. Your code just doesn't work at the moment.

[CNelias]

@Datseris The tests are now passing correctly ! I reused your

pitch_to_fundamental(notes) = [MIDI.PITCH_TO_NAME[mod(n.pitch, 12)] for n in notes]

I could not have come up with the idea myself since the attribute PITCH_TO_NAME is referenced nowhere in the doc .

[Datseris]

Alright, thanks for the effort! I'll properly review this over the weekend and if all is proper I will merge.

and I'll add PITCH_TO_NAME to the docs.