Added genre to ballroom datataset loader #629

Masetto96 · 2024-04-30T13:20:47Z

The track object of the ballroom dataset does not have the annotation about the genre of the track. Therefore, the genre attribute have been included from the name of the audio path.

Test for getting the genre from the audio path has been implemented.

Following the contribution guidelines, code has been formatted and all tests passed.

Masetto96 · 2024-04-30T13:23:59Z

@genisplaja @guillemcortes plz review :)

genisplaja · 2024-10-10T16:58:46Z

hey @Masetto96, I did finally take a look at this one and LGTM. Let us update #630 so that codecov works as expected and we can try to merge this one.

genisplaja

Right! Almost there, I guess it works as expected, and I don't see any missing tests. Just requesting minor changes for consistency and that's ready :)
Sorry for taking that long to review... we were super busy updating soundata for the JOSS submission man. Thanks for the understanding!

tests/test_loaders.py

genisplaja · 2024-10-10T17:00:39Z

mirdata/datasets/ballroom.py


    Cached Properties:
        beats (BeatData): human-labeled beat annotations
        tempo (float): human-labeled tempo annotations
+


remove added enter. Or is it because formatting issues?

Removed the new line, however I noticed that in other scripts it is sometimes present

Ok! Was just not to unnecessary change lines. I guess it does not affect at all, I can take a look

genisplaja · 2024-10-10T17:02:13Z

mirdata/datasets/ballroom.py

@@ -164,6 +168,20 @@ def to_jams(self):
        )


+def get_genre(audio_path: Optional[str]) -> Optional[str]:


normally, for consistency, we name this functions load_<thing>(). I think it would be nice if you could use this convention here as well

Masetto96 · 2024-10-11T08:11:24Z

mirdata/datasets/ballroom.py

@@ -130,6 +132,8 @@ def __init__(
        self.beats_path = self.get_path("beats")
        self.tempo_path = self.get_path("tempo")

+        self.genre = get_genre(self.audio_path)


While making the changes I spotted a mistake. Here I changed line 135 to self.load_genre()

Yes! @Masetto96 although are you sure is self.load_genre()? I think that the load_genre() function is outside the class right? Shouldn't it be without the self.?

Oops, you are right! I assumed it was part of the class but I did not look carefully. Sorry about that!

Masetto96 · 2024-10-11T08:12:04Z

tests/datasets/test_ballroom.py

+    data_home = "tests/resources/mir_datasets/ballroom"
+    dataset = ballroom.Dataset(data_home)
+    track = dataset.track("Media-105901")
+    genre = ballroom.get_genre(track.audio_path)


Made necessary changes also here

genisplaja

LGTM! I guess we need PR #631 to be merged so that the tests pass, and then we can propagate the update to this PR and merge. Thanks @Masetto96 :)

genisplaja · 2024-10-16T14:09:20Z

hey @Masetto96, very close but there is a small thing that we are still missing. All custom load_* methods are passed through a test to check that if a wrong filepath is given, an error should be thrown, but in your load_genre function that does not happen, and that's why the tests are not passing. I guess all files should have a genre right? Because all audio files are contained in a "genre"-named folder. Then it would make sense that an error is thrown if the audio path is not found.

Then, you would need to see if the filepath exists, and throw, I think, a FileNotFound error otherwise.

… throwing error if file is not present

Masetto96 · 2024-10-16T15:19:38Z

Done! Let me know if I there is something else. Thank you for the code review.

genisplaja · 2024-10-17T11:58:15Z

Hey @Masetto96 man, finally fetched your fork and played around to figure out the issue. Sorry that I didn't notice that we were using a too overcomplicated solution for a problem that should be way easier to address. Let me explain how we should be actually loading the genre:

The load_genre method should not exist.
You can directly get self.genre as an attribute of the Track class like this: self.genre = os.path.basename(os.path.dirname(self.audio_path)).lower(), in the Track init.
Then, the test_load_genre is also not needed anymore.

In the end, the genre annotation is not contained in a local file, but rather in a filepath that is already contained in the library itself (in the .json index), so you wouldn't even need to download the dataset to get the genre. Therefore, we don't need a cached property for that. genre can be a simple attribute of the ballroom Track class and you would read from the path. Sorry that I did not notice that earlier... thanks for following my suggestions man. If you could update that, I am pretty sure the tests will pass (are passing locally for me), so we will be able to move on with that one. Thanks again and sorry for the confusion :)

…om path name in the init

Masetto96 · 2024-10-21T10:29:44Z

Of course @genisplaja , your suggestion makes a lot of sense!

codecov · 2024-10-21T10:32:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.08%. Comparing base (dc3ac00) to head (8f8a3ae).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
+ Coverage   97.07%   97.08%   +0.01%     
==========================================
  Files          64       64              
  Lines        7392     7388       -4     
==========================================
- Hits         7176     7173       -3     
+ Misses        216      215       -1

Masetto96 added 2 commits April 30, 2024 15:01

added genre to track obj of ballroom loader, added test, updated docs

f0d2360

removed unused import

24693af

genisplaja requested changes Oct 10, 2024

View reviewed changes

minor changes for consistency

a1ca374

Masetto96 commented Oct 11, 2024

View reviewed changes

Masetto96 added 2 commits October 11, 2024 10:13

minor change to ballroom tests to ensure consistency

7b958a6

fix load_genre function calling outside of class

1020ff5

genisplaja approved these changes Oct 12, 2024

View reviewed changes

Merge branch 'master' into master

d1dac3f

adapted ballroom/load_genre func to coerce_to_bytes_io, automatically…

37118f3

… throwing error if file is not present

Masetto96 added 2 commits October 16, 2024 17:20

removed code commented out

e366389

black formatted

086a12c

genre attribute of ballroom is no longer a cached property, loaded fr…

2e0796d

…om path name in the init

Merge branch 'master' into master

8f8a3ae

tanmayy24 merged commit 73edbf9 into mir-dataset-loaders:master Oct 28, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added genre to ballroom datataset loader #629

Added genre to ballroom datataset loader #629

Masetto96 commented Apr 30, 2024

Masetto96 commented Apr 30, 2024

genisplaja commented Oct 10, 2024

genisplaja left a comment •

edited

Loading

genisplaja Oct 10, 2024

Masetto96 Oct 11, 2024

genisplaja Oct 11, 2024

genisplaja Oct 10, 2024

Masetto96 Oct 11, 2024

genisplaja Oct 11, 2024 •

edited

Loading

Masetto96 Oct 11, 2024

Masetto96 Oct 11, 2024

genisplaja left a comment

genisplaja commented Oct 16, 2024 •

edited

Loading

Masetto96 commented Oct 16, 2024

genisplaja commented Oct 17, 2024

Masetto96 commented Oct 21, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

		@@ -164,6 +168,20 @@ def to_jams(self):
		)


		def get_genre(audio_path: Optional[str]) -> Optional[str]:

Added genre to ballroom datataset loader #629

Added genre to ballroom datataset loader #629

Conversation

Masetto96 commented Apr 30, 2024

Masetto96 commented Apr 30, 2024

genisplaja commented Oct 10, 2024

genisplaja left a comment • edited Loading

Choose a reason for hiding this comment

genisplaja Oct 10, 2024

Choose a reason for hiding this comment

Masetto96 Oct 11, 2024

Choose a reason for hiding this comment

genisplaja Oct 11, 2024

Choose a reason for hiding this comment

genisplaja Oct 10, 2024

Choose a reason for hiding this comment

Masetto96 Oct 11, 2024

Choose a reason for hiding this comment

genisplaja Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Masetto96 Oct 11, 2024

Choose a reason for hiding this comment

Masetto96 Oct 11, 2024

Choose a reason for hiding this comment

genisplaja left a comment

Choose a reason for hiding this comment

genisplaja commented Oct 16, 2024 • edited Loading

Masetto96 commented Oct 16, 2024

genisplaja commented Oct 17, 2024

Masetto96 commented Oct 21, 2024 • edited Loading

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

genisplaja left a comment •

edited

Loading

genisplaja Oct 11, 2024 •

edited

Loading

genisplaja commented Oct 16, 2024 •

edited

Loading

Masetto96 commented Oct 21, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading