Skip to content
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

Column locator sequence input #415

Merged
merged 8 commits into from
Jun 19, 2024
Merged

Conversation

ChristianGeng
Copy link
Member

@ChristianGeng ChristianGeng commented May 24, 2024

Goal

close #407: Typing provided squence type input to Dependencies._column_loc but it wasn't implemented.

Solution

There was a mismatch between the type hints suggesting that it is possible to use sequence input and output.
We have chosen to update the type hints as the suggested vectorized implenentation did not provide large benefits.

Therefore this corrects the type hints allowing only scalar in- and putput.

@ChristianGeng ChristianGeng requested a review from hagenw May 24, 2024 08:21
tests/test_dependencies.py Outdated Show resolved Hide resolved
Copy link
Member

@hagenw hagenw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for trying to tackle this.

The wrong typing most likely comes from my test supporting lists instead of scalars to speed processing up in #370, but as I was not able to find a meaningful speed up, I reverted the changes in #393.

I would suggest to solve #407 by changing the typing to support only scalars.

audb/core/dependencies.py Outdated Show resolved Hide resolved
audb/core/dependencies.py Outdated Show resolved Hide resolved
tests/test_dependencies.py Outdated Show resolved Hide resolved
Christian Geng and others added 6 commits May 29, 2024 16:56
Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>
Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>
Co-authored-by: Hagen Wierstorf <hwierstorf@audeering.com>
@ChristianGeng ChristianGeng force-pushed the column-locator-sequence-input branch from 8063281 to 80289a1 Compare May 29, 2024 14:57
@hagenw
Copy link
Member

hagenw commented May 30, 2024

Thanks for updating the code. Can you also update the description of the pull request, so it's easy to see later on, what was added here.

@hagenw
Copy link
Member

hagenw commented May 30, 2024

BTW, I checked how we avoid calling [deps.bit_depth(file) for file in files] when we want to get the information for all files in audb.info.bit_depth(), and the trick is to just get the column from the dependency table:

return set(df[df.type == define.DependType.MEDIA].bit_depth)

Copy link
Member

@hagenw hagenw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated in #415 (comment) this is ready to merge, but would be nice to first update the description.

@ChristianGeng
Copy link
Member Author

As stated in #415 (comment) this is ready to merge, but would be nice to first update the description.

I have updated the PR description above. Merging.

@ChristianGeng ChristianGeng merged commit 667d169 into main Jun 19, 2024
9 checks passed
@ChristianGeng ChristianGeng deleted the column-locator-sequence-input branch June 19, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependencies._column_loc: files parameter has a mismatch between typing and implementation
2 participants