Refactor channel_group_search implementation in tdm_loader #27
Conversation
When searching and resulting groups has identical channel group names, only the first channel group index is returned New logic will look at channel group names together with occurrences to find the correct channel group index.
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
The previous implementation counted and used the number of times a channel group name appeared, which added extra complexity to the search logic. The new logic simplifies this process by directly appending the names to the found_terms list and by making the search case- and space-insensitive.
Switched from a for-loop and conditional check structure to an inline list comprehension to search for channel group names. The aim of this refactor is to simplify the code and improve readability, while maintaining the functionality of ignoring None values and retaining valid group names.
|
The latest commit I have used yours suggestion and removed my implementation to find the occurrences with the for loop. |
Changed the return type of a function in tdm_loader from a list to a set. This modification ensures unique channel group names are returned from the function, thus improving the accuracy of the channel search process.
|
Thank you for the changes. One of the tests are not passing now, because you changed the return type of the function from list to set. I think it would also be useful to not use a set here as people might expect that this function returns a list. I would suggest that you do a check whether a certain key already exists before inputting it into the list (or you use a set in the for-loop and convert it to a list in the return but you might need to sort it to pass the test again). Do you also have a file containing duplicate names which we could add as a test for your new feature? |
|
Sorry for my clumsiness, this is my first contribution ever. I did not have any small TDM/TDX files I could share to make a test, all my files are 500mb ++ and Autogenerated from a recorder. |
No worries. This is totally fine and part of the review process. Thank you for the changes again. Also congrats on your first contribution.
Ok this is too large unfortunately. There is not chance you are able to generate a very small testing file just containing just some duplicate axes? |
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
Co-authored-by: Florian Dobener <github@schroedingerscat.org>
|
suddenly it became such an little fix :) |
|
Thank you @EspenEnes. I'll merge it now and create a new release with it so you can use it |
When searching and resulting groups has identical channel group names, only the first channel group index is returned
New logic will look at channel group names together with occurrences to find the correct channel group index.