Easy Task #4
Easy Task #4
Conversation
roszcz
left a comment
roszcz
left a comment
There was a problem hiding this comment.
Thank you, very nice!
Your code is very clean, but, like I hinted in one of the comments, I would say that having most functions with doc-strings longer than the code is a sign of over documentation. We read code sequentially, and having to jump between functions every two lines breaks the flow, at least for me :)
speed.py
Outdated
| def count_notes_in_interval(record: pd.DataFrame, interval: pd.Interval) -> int: | ||
| """ | ||
| Counts the number of notes played in a given interval. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| record : pd.DataFrame | ||
| DataFrame containing data about the notes played | ||
| interval : pd.Interval | ||
| Interval in question | ||
|
|
||
| Returns | ||
| ------- | ||
| int | ||
| Number of notes played in the given interval | ||
| """ | ||
| return record.loc[(record["start"] >= interval.left) & (record["end"] < interval.right)]["pitch"].count() |
There was a problem hiding this comment.
In terms of clean code, my opinion here is that this shouldn't be a separate function . You created 15 lines of documentation for a single line of code, but the documentation only describes inputs and outputs - it doesn't tell me anything about what the code is actually doing. Here's an alternative approach:
# For notes within the interval ...
ids = (record.start >= interval.left) & (record.end < interval.right)
# ... count all the pitches
pitch_count = record[ids]["pitch"].count()And I would keep it like that in count_notes_by_intervals, even at the cost of breaking L137 into 4 or 5 lines.
Again, this is my opinion, there's no difference in logic, only in the way it reads. At the same time I think that readability is the most important factor when working on complex projects, and even small improvements can make a huge difference in the long term :)
There was a problem hiding this comment.
Moved the functionality to the other function.
|
And don't worry about ffmpeg - animation functionality of |
|
Started working on the chords. |
2 participants
Solved the first part. Found the fastest 15 seconds of music.
fastest.mp4
To reproduce results run
python -m fastest --duration 15 --dataset-uri roszcz/maestro-v1 --fig-path figure.png --vid-path video.mp4.I myself am having trouble reproducing the video portion of the MIDI since I updated my system and it seems to have broken ffmpeg for me.