Fix Database.update() for misc tables#467
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.
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: