Quantisation of events and notes #327

superbock · 2017-08-22T11:16:58Z

Changes proposed in this pull request

Quantisation of events does not alter them
Check for correct dimensionality (i.e. 1D) of input data
Add functionality to be able to quantise notes, i.e. return a 2D representation of the notes.

This pull request fixes #325.

fdlm

I guess the code is fine, but I suggest some changes to documentation and the function signature.

fdlm · 2017-09-18T07:42:04Z

madmom/utils/__init__.py

+def quantize_notes(notes, fps, length=None, num_notes=None, shift=None,
+                   offset=None, velocity=None):
+    """
+    Quantize the notes with the given resolution.


I think the description needs to be elaborated. I would expect that "quantizing the notes" would "snap" their start and end points to a grid. This function, however, does something else. I'm not sure how to describe this concisely...

Added a more descriptive docstring.

fdlm · 2017-09-18T07:45:36Z

madmom/utils/__init__.py

+    length : int, optional
+        Length of the returned array. If 'None', the length will be set
+        according to the latest sounding note.
+    num_notes : int, optional


The name of this parameter is easily confused with the "number of notes in the notes array". Maybe changing the description to something like "Number of possible pitches"? Not sure...

Renamed to num_pitches.

fdlm · 2017-09-18T07:47:08Z

madmom/utils/__init__.py

+        will be based on the highest note number in the `notes` array. All note
+        numbers equal or greater this value will not be included in the
+        returned quantized array.
+    shift : float, optional


I don't think shift or offset should be part of this function.

It is very easy to implement both outside of the function

It makes the function signature unnecessarily complex

True, at least for offset. On the other hand, since shift can be a float and rounding is used inside the function, moving this outside the function may introduces more errors.

IMHO the following is not more error-prone:

notes[:, 0] += 2.3 qnotes = quantize_notes(notes)

than this:

qnotes = quantize_notes(notes, shift=2.3)

If there is a common use case for the shift, then the second option is fine, but if it is a special case, I vote for the first one, because I think a function should do only one thing, if possible. That said, I don't have a strong opinion on this specific case.

You are right, I somehow only considered applying the shift after calling the function... I will remove it (and deprecate shift in quantize_events).

fdlm · 2017-09-18T13:12:33Z

madmom/utils/__init__.py

+    If no `length` and `num_pitches` are given the values are inferred from the
+    notes. Then each note is put into the column corresponding to its pitch
+    and all frames between the note's onset and offset (i.e. onset + duration)
+    are set to the velocity of the note.


Already much better! I would try to describe what the function does, and not how it does it, maybe like this:

"Create a sparse 2D array whose rows correspond to points in time (according to fps and length), and columns to note pitches (according to num_pitches). The values of the array correspond to the velocity of a pitch at a given point in time (based on the note pitch, onset, duration, and velocity). If no values for length and num_pitches are given, they are inferred from notes."

fdlm · 2017-09-18T13:13:06Z

madmom/utils/__init__.py

+        Number of pitches of the returned array. If 'None', the number of
+        pitches will be based on the highest pitch in the `notes` array.
+    shift : float, optional
+        Shift the events by `shift` seconds before quantization.


If we keep the shift parameter, I think it should say "Shift the notes...

fdlm · 2017-09-18T13:15:13Z

I think there was a comment of yours about renaming of the function to expand_notes, or something like that. I can't find it right now. Anyways, I think with a useful description in the docstring, both names should be fine. I would go with whatever is more consistent with the rest of the codebase.

superbock force-pushed the fix_325 branch 3 times, most recently from 5f42a17 to 7ebc0ff Compare August 22, 2017 13:55

superbock mentioned this pull request Aug 22, 2017

Quantize Events - Wrong dimensions returned. #325

Closed

superbock force-pushed the fix_325 branch 4 times, most recently from f3be7c1 to f510251 Compare September 17, 2017 11:29

superbock changed the title ~~Quantisation of events does not alter them~~ Quantisation of events and notes Sep 17, 2017

superbock requested a review from fdlm September 17, 2017 11:34

superbock force-pushed the fix_325 branch 4 times, most recently from 5525cfc to 7f39a86 Compare September 18, 2017 03:47

fdlm suggested changes Sep 18, 2017

View reviewed changes

superbock force-pushed the fix_325 branch 2 times, most recently from 02e69ee to 19a2530 Compare September 18, 2017 12:37

fdlm reviewed Sep 18, 2017

View reviewed changes

superbock force-pushed the fix_325 branch 2 times, most recently from 98cf7ec to a116526 Compare September 18, 2017 15:27

fdlm approved these changes Sep 19, 2017

View reviewed changes

Sebastian Böck added 4 commits September 19, 2017 11:22

quantization of events does not alter them

f345041

add input dimensionality checks

25b1657

add quantize_notes function

9fad07c

deprecate shift parameter of quantize_events

7b36ee2

superbock force-pushed the fix_325 branch from a116526 to 7b36ee2 Compare September 19, 2017 09:23

superbock merged commit 0e778b9 into master Sep 19, 2017

superbock deleted the fix_325 branch September 19, 2017 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quantisation of events and notes #327

Quantisation of events and notes #327

superbock commented Aug 22, 2017 •

edited

Loading

fdlm left a comment

fdlm Sep 18, 2017

superbock Sep 18, 2017

fdlm Sep 18, 2017

superbock Sep 18, 2017

fdlm Sep 18, 2017

superbock Sep 18, 2017

fdlm Sep 18, 2017

superbock Sep 18, 2017 •

edited

Loading

fdlm Sep 18, 2017

fdlm Sep 18, 2017

fdlm commented Sep 18, 2017

Quantisation of events and notes #327

Quantisation of events and notes #327

Conversation

superbock commented Aug 22, 2017 • edited Loading

Changes proposed in this pull request

fdlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superbock Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdlm commented Sep 18, 2017

superbock commented Aug 22, 2017 •

edited

Loading

superbock Sep 18, 2017 •

edited

Loading