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

Refactor channel search functionality in tdm_loader #28

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

EspenEnes
Copy link
Contributor

The previous implementation of the channel search function in the tdm_loader.py was refactored. Search now matches channels directly without set generation and updates retrieval with more efficient lookups. The modification updates the testing to reflect the changes in the returned search results as well.

Before the refactor searching throe 22000 channels took 6.5min and after it took 0.08s

Also the test has changed to be similar to group_search when searching for "", all channels should be returned.

The previous implementation of the channel search function in the `tdm_loader.py` was refactored. Search now matches channels directly without set generation and updates retrieval with more efficient lookups. The modification updates the testing to reflect the changes in the returned search results as well.

Before the refactor searching throe 22000 channels took 6.5min and after it took 0.08s

Also the test has changed to be similar when searching for "", all channels should be returned.
Copy link
Owner

@domna domna left a comment

Choose a reason for hiding this comment

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

Thank you for yet another fix :)

It's looking all good but I suggest to use a caching function. It's a bit more pythonic and easier to understand for people reading the code (and is essentially doing what you did "manually").

tdm_loader/tdm_loader.py Outdated Show resolved Hide resolved
tdm_loader/tdm_loader.py Outdated Show resolved Hide resolved
tdm_loader/tdm_loader.py Outdated Show resolved Hide resolved
tdm_loader/tdm_loader.py Outdated Show resolved Hide resolved
tdm_loader/tdm_loader.py Outdated Show resolved Hide resolved
tdm_loader/tdm_loader.py Outdated Show resolved Hide resolved
EspenEnes and others added 7 commits March 25, 2024 12:15
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
A cache decorator is introduced for the `get_channels` function to speed up repeated lookups, replacing the previous, dictionary lookup.
@EspenEnes
Copy link
Contributor Author

Thanks, I have never used the cache tool before, I knew about it, but never used it.
This was a great suggestion.

But should the function be privat?

@EspenEnes EspenEnes requested a review from domna March 25, 2024 11:55
@domna
Copy link
Owner

domna commented Mar 25, 2024

Thanks for the changes!

But should the function be privat?

Yes, I think this would be good. You can also nest it into the channel_group_search. That way it's not accessible anywhere outside this function (as private functions are still accessible but by convention shouldn't be used). But naming it _get_channels is also fine with me.

The public `get_channels` function is renamed to the private method `_get_channels` in 'tdm_loader.py'. This is due to internal use only, to provide more clarity and prevent unintended external access.
The method name "get_channels" has been corrected to "_get_channels" in the 'tdm_loader.py' file. The change was necessitated by the requirement for the method to be private, hence the underscore, as it's only meant for internal use, preventing unintended external access.
@EspenEnes
Copy link
Contributor Author

Should the channel search also use the description of the channel when searching? Or should that be a different function.

@domna
Copy link
Owner

domna commented Mar 26, 2024

Should the channel search also use the description of the channel when searching? Or should that be a different function.

I think it's better to have a separate function for it or at least a function parameter which defaults to false.

Copy link
Owner

@domna domna left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the contribution :)

@domna domna merged commit 9d431e4 into domna:master Mar 28, 2024
1 check passed
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.

3 participants