Cleans up & renames swar2freq() method as swar_to_frequency() #11
Conversation
|
Feedback is welcome for my approach. cc @amoghpj @reckoner165 馃槂 |
| @@ -4,82 +4,98 @@ | |||
| import struct | |||
| import pyaudio | |||
|
|
|||
| def swar2freq(swar,basefreq): | |||
| SWAR_FREQUENCY_MAPPER = { | |||
There was a problem hiding this comment.
Wondering if we should move SWAR_FREQUENCY_MAPPER and VALID_SWAR_INPUT subsets into a separate notes.txt or so since they're constants.
There was a problem hiding this comment.
Keeping them as a global dict and set (respectively) is better than loading them from a text / JSON file as the latter involves loading of files which in turn involves more computation (comparitively) than the former. 馃槄
|
|
||
| } | ||
|
|
||
| """ | ||
| For any other input, including captialization, this function |
There was a problem hiding this comment.
I was hoping to use capitalization for some thing else. Not sure right now, but I might consider using capital letters for shuddh swars and small letters for komal/teevr swars.
| first_swar = swar[0] | ||
| is_swar_valid = first_swar in VALID_SWAR_INPUTS | ||
| if not is_swar_valid: | ||
| return SWAR_FREQUENCY_MAPPER.get('0') |
There was a problem hiding this comment.
Never seen get() before. Learned something new! Thanks!
There was a problem hiding this comment.
There are two reasons why dict.get() is used to fetch data -
- If a key is absent in
dict[key], it will raise aKeyErrorwhich needs to be handled downstream by atry-exceptblock. - You can specify a fallback value -
For example if the name "Foo" is unavailable in a dictBABY_NAMES(not a great baby name, I know 馃槤), thenBABY_NAMES.get("Foo")would returnNone. If you have a unique fallback say "Bar", you can specify that like thisBABY_NAMES.get("Foo", "Bar").dict.get(key, fallback_value)is what is recommended by experienced developers for better readability and debuggability (I think I made this word up).
dict.get(key, fallback_value) is the same as
value = dict.get(key)
if value is None:
value = <default_value>
| return int(shuddh_frequency * base_frequency_float) | ||
|
|
||
| if swar_length == 2: | ||
| if first_swar in VALID_SWAR_INPUTS_SUBSET_1: |
There was a problem hiding this comment.
variable name first_swar doesn't make sense. The first character (< or >) specifies the octave. Call this octave specifier maybe? @onstash
There was a problem hiding this comment.
By all means. I called it first_swar because it was the first entity in swar list. 馃槄
| '0': 0, | ||
| } | ||
| VALID_SWAR_INPUTS_SUBSET_1 = set(['<', '>']) | ||
| VALID_SWAR_INPUTS_SUBSET_2 = set(['s', 'r', 'g', 'm', 'p', 'd', 'n']) |
There was a problem hiding this comment.
This definitely seems very systematic, but can you explain why this needs to be done this way? The type conversions from list to set to list back to set?
There was a problem hiding this comment.
I read somewhere that looking up entities in a set is faster than looking up entities in a list. Unfortunately, I am unable to find the blog post / stackoverflow discussion link related to this. On the other hand, the gain from the "lookup is faster in set" theory might be notable if the data size is large enough. We're operating with < 100 entities. In that case, this is an unnecessary premature optimization. I will revert this list to set conversion as it has introduced confusion and possibly premature optimization instead of solving a (non-existent) performance issue. Sorry for the confusion. 馃槄
|
@amoghpj @reckoner165 Please point out where variable names don't make sense from a musical perspective. |
This cleaning up and renaming was done keep readability and performance in mind. It is more of an enhancement. 馃槃