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

feat: Support for setting aggregation for (Hybrid)RouteLayer #202

Merged
merged 11 commits into from
Mar 15, 2024

Conversation

andreped
Copy link
Contributor

fixes #201.

Also fixed:

  • Added unit tests to verify that aggregation selection updates the aggregation
  • Added unit tests to verify that aggregation works as intended for both RL and HybridRL
  • Added support to set top_k for RouteLayer similar to HybridRouteLayer
  • Added unit tests for top_k for RouteLayer
  • Added some error handling when setting both top_k and aggregation
  • Linted code

@andreped andreped changed the title feat: Support for setting aggregation method for (Hybrid)RouteLayer feat: Support for setting aggregation for (Hybrid)RouteLayer Mar 14, 2024
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 78.00%. Comparing base (1394adf) to head (7cd3755).

Files Patch % Lines
semantic_router/layer.py 82.35% 3 Missing ⚠️
semantic_router/hybrid_layer.py 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
+ Coverage   77.96%   78.00%   +0.04%     
==========================================
  Files          42       42              
  Lines        2119     2146      +27     
==========================================
+ Hits         1652     1674      +22     
- Misses        467      472       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andreped
Copy link
Contributor Author

andreped commented Mar 14, 2024

@jamescalam Just realized that the codecov.xml file is inside the repo. IMO, it does not make any sense to have there. Code coverage is viewed in an external website (see here), which then is linked to this badge in the README, right?

So can it be deleted? I can do this now in this PR, if of interest, as well as add codecov.xml to .gitignore.

@andreped
Copy link
Contributor Author

I also see that codecov complains about 5 lines above, but they are all related to error handling. Do I need to add these in tests or should I add #nopragma or similar?

Copy link
Member

@bruvduroiu bruvduroiu 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 the contribution! I agree that only using sum tends to favor Routes with more utterances.

Very small nitpick on the choice of naming the aggregation strings. We want to keep in line with the Pandas lowercase aggregation function names.

@bruvduroiu bruvduroiu added the enhancement Enhancement to existing features label Mar 15, 2024
@andreped
Copy link
Contributor Author

andreped commented Mar 15, 2024

Just ran tests with this in one of my projects. Works fine! :] But of course, as a top_k of 2 or 3 worked best for my project, the aggregation method was less important. Then again for higher values of top_k I got a change in performance.

@bruvduroiu bruvduroiu self-requested a review March 15, 2024 11:20
@bruvduroiu bruvduroiu merged commit 863032e into aurelio-labs:main Mar 15, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for different aggregation in (Hybrid)RouteLayer voting?
3 participants