Conversation
|
Thanks @lylyhan ! Can you fix the merge conflicts before we start a code review? Either way of doing it is fine (web editor or command line). Let me know if you need assistance getting it sorted out. |
|
OK! i revised all suggestions above (replaced already implemented functions, revised __test_xx_filter series, corrected the messed up merge), pytest now pass on all functions. |
| __all__ = ["Filter", "RandomLPFilter", "RandomHPFilter","RandomBPFilter"] | ||
|
|
||
|
|
||
| def checkfreqinband(freq,state,datatype): |
There was a problem hiding this comment.
Can you add a docstring for this function, explaining what its inputs should be and what it returns? See _get_rng for an example of how that sort of thing should look.
| ) | ||
|
|
||
| @staticmethod | ||
| def filter_class(annotation, state): |
There was a problem hiding this comment.
I think we should leave pitch_class annotations unchanged. Pitch classes don't have octave information, so it's impossible to tell what frequency they actually correspond to, and whether or not they lie in the passband of the filter.
There was a problem hiding this comment.
I checked https://jams.readthedocs.io/en/stable/namespace.html#pitch-class again and seems like the "tonic" and "pitch" keys together did specify octave information?(for example C7 is convertible to a specific frequency in hz)
muda/deformers/filter.py
Outdated
|
|
||
| Examples | ||
| -------- | ||
| >>> # Shift down by a quarter-tone |
There was a problem hiding this comment.
comment seems incorrect / copypasta here
muda/deformers/filter.py
Outdated
| self.order = order | ||
| self.attenuation = attenuation | ||
| if self.btype == "bandpass": | ||
| if type(cutoff) == tuple: |
There was a problem hiding this comment.
checking type(x) == y is an anti-pattern.
Instead, say isinstance(cutoff, tuple). (Similarly for following lines)
| if all(len(i) == 2 for i in cutoff): # [[a,b],[c,d]] | ||
| self.cutoff = [tuple(c) for c in cutoff] | ||
| else: | ||
| raise ValueError("bandpass filter cutoff must be tuple or list of tuples") |
There was a problem hiding this comment.
why not check this first (outside of checking btype) so you don't have multiple copies of this exception?
There was a problem hiding this comment.
i designed the cutoff variable format to be btype-specific (when btype == bandpass, take list of tuples, tuple or list of length-2 lists; when btype==others, take list of floats or one float), so i can't really check its format outside of knowing what btype it is.. The repetitive exception is each corresponding to a different case: first one is when input is a list of lists of not necessarily length 2, second one is when input is a list of neither tuples nor lists, third one is when input is neither a list nor a tuple. Maybe i should change the name of the exception accordingly to avoid appearing to be repetitive?
muda/deformers/filter.py
Outdated
|
|
||
| Examples | ||
| -------- | ||
| >>> # Shift down by a quarter-tone |
muda/deformers/filter.py
Outdated
|
|
||
| Examples | ||
| -------- | ||
| >>> # Shift down by a quarter-tone |
muda/deformers/filter.py
Outdated
| raise ValueError("n_samples must be None or positive") | ||
|
|
||
| if order <= 0 or attenuation <= 0: | ||
| raise ValueError("order and attenuation must be None or positive") |
There was a problem hiding this comment.
if None is a valid value, checking order <= 0 will raise a type error. You need to handle Nones separately:
if order is not None and order <= 0:
...It's also not good style to combine two tests and then raise a single exception, because a user then has to figure out which of the two tests failed. Better to factor these out into separate conditional statements.
2 participants
added lowpass, highpass, bandpass, randomized filtering functionalities, and their corresponding testing functions