-
Notifications
You must be signed in to change notification settings - Fork 1
Fix Database.update() for misc tables #467
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
Conversation
Reviewer's GuideEnhanced Database.update() to correctly merge miscellaneous tables by checking all existing table IDs and added a targeted test to cover this scenario. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
99df383 to
f9c53db
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
🚀 New features to boost your workflow:
|
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.
Hey @hagenw - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
One thing I realized when debugging the existing test is that db["misc"].df= float idx
a 0.137095
b 0.783854
other1["misc"].df= float labels
idx
b 0.281209 c
c 0.875555 bBut after the first failed attempt we have: which means But this is most likely another issue and not related to what we try to fix here. |
I had a look and I found this line here. Here we see that the index of the table that is used to updated is indeed extended. I agree that this is a somehow unexpected behavior. I created #469. |
The test function that currently covers |
|
Since the suggested change fixes the issue I will merge now. |
Yes, during the last year I also figured out that there are better ways to write long tests that depend on a status of an object like a database. One way is to use a class which ensures that the status of the database is the same for each of the single tests. One example can be found at https://github.com/audeering/audbcards/blob/a6a5cc1b356633965c1e39a1200cb61a5aae7c0b/tests/test_dataset.py#L507-L588. |
Closes #466
Fixes
audformat.Database.update()for the case of misc tables. The error was not covered by our previous test. I'm still not completely sure why, but to save time I added a new test for the particular problem.The error was due to us looking only at
self.tablesfor existing tables in the database that should be updated. I changed it tolist(self)to include misc tables as well.Summary by Sourcery
Fix misc table handling in Database.update by including them in the update loop and add a corresponding test.
Bug Fixes:
Tests: