-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] OpenMIC2018 #544
[WIP] OpenMIC2018 #544
Conversation
I think I don't understand why the loader tests are failing / what the expected behavior is here: Lines 323 to 355 in e591c54
(Incidentally, this would be a bit cleaner using a fixture to iterate over the datasets, rather than sticking them all in a loop inside the test. You could cut down on prints this way and isolate failures.) |
Still not making any headway on the loader tests. The placeholder test fails because it's trying to load metadata from a path that doesn't exist: @core.cached_property
def _metadata(self):
metadata_path = Path(self.data_home) / "openmic-2018-metadata.csv"
try:
with open(metadata_path, "r") as fdesc:
# index column is second to last
metadata = pd.read_csv(fdesc, index_col=-2)
except FileNotFoundError as exc:
> raise FileNotFoundError(
f"Metadata file {metadata_path} not found. " "Did you run .download?"
) from exc
E FileNotFoundError: Metadata file not/a/real/path/openmic2018/openmic-2018-metadata.csv not found. Did you run .download? This seems to me like exactly what should happen? Clearly I'm missing something here: why is this expected to work? |
Bumping this one -- lil help? |
Hello @bmcfee! The problem is in
The problem is in loading the metadata file in the Track class init. The issue could be fixed by converting this |
Thanks @genisplaja, that's easy enough to fix. I still don't understand what this test is trying to accomplish though: it seems like it ought to be an intentional failure. |
Codecov Report
@@ Coverage Diff @@
## master #544 +/- ##
==========================================
+ Coverage 96.59% 96.61% +0.02%
==========================================
Files 48 49 +1
Lines 5908 6004 +96
==========================================
+ Hits 5707 5801 +94
- Misses 201 203 +2 |
Might be a good time to revive this one.. is there anything that needs doing from my side here? |
Attempting to bump this one again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the delay, Brian. this looks good. I have just added some small questions regarding the way you construct the paths.
CI failures here seem to be unrelated to this PR. I've fetched upstream and rebased, so not sure what's going on here (unless master is broken). |
there is an error in one of the existing tests in the master branch indeed. I will look into it this week. |
I have fixed the problem on the main branch. so you can update this PR with the master |
if no one objects, I propose merging this into main branch. pandas is already installed as a dependency of jams |
Thanks for merging this - I do think there are still some unresolved issues to clear up as noted in my original post:
|
hi Brian! |
@nkundiushuti to be honest, this PR languished for so long that I don't have a clear recollection of the issues. For (3), I don't think I ever fully understood what the tests were, or what else could/should be added. I think this should be more on the maintainers to decide rather than contributors. For (1), the question is pretty well described in #253 - I'd suggest re-opening that issue until it's entirely resolved. There are also some other finnicky points in #253 that I raised for discussion, but never received any feedback on (eg erroring out when users request random splits). |
This PR adds a dataset class for openmic2018. Fixes #253
The basic functionality is there, but there are a few things left to sort out: