Skip to content

Add recommendation breakdown function #4188

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

Merged
merged 9 commits into from
May 25, 2023
Merged

Conversation

eccabay
Copy link
Contributor

@eccabay eccabay commented May 24, 2023

Closes #4187

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #4188 (88b38df) into main (fd3654c) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #4188     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        349     349             
  Lines      38107   38128     +21     
=======================================
+ Hits       37990   38011     +21     
  Misses       117     117             
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.6% <100.0%> (+0.1%) ⬆️
evalml/tests/automl_tests/test_automl.py 99.6% <100.0%> (+0.1%) ⬆️

@eccabay eccabay marked this pull request as ready for review May 24, 2023 14:14
Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

LGTM but wondering if we should include the score weights as well!

@@ -1714,6 +1714,30 @@ def full_rankings(self):
rankings_df.reset_index(drop=True, inplace=True)
return rankings_df

def get_recommendation_score_breakdown(self, pipeline_id):
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we include the score weights as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaning towards not - the score weights are known and/or set ahead of time, while the scores themselves are not. I'd rather keep this simpler and more understandable, but I am open to adding something if you disagree!

Copy link
Collaborator

Choose a reason for hiding this comment

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

where do we display the weights to the user? I don't see anything here so far. If the purpose of get_recommendation_score_breakdown is to explain to the user how the recommendation score is calculated, I think we should give them the weights so they know how they go from the metrics to the recommendation score.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The weights aren't directly displayed to the user, but the docstring states that they are weighted equally by default and we offer methods for customization by the user.

The goal of this function isn't to explicitly explain to the user how the recommendation score was calculated - that's why the values reported are explicitly the non-normalized ones. Rather, the goal is to present the user with the evaluated objectives behind the recommendation score. What they do with that information is up to them, but it's more to give them the insight about direct model performance rather than "here's the equation".

Copy link
Collaborator

Choose a reason for hiding this comment

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

works for me!

Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

This looks good and looks like it will play nicely with the further stories to update this functionality. Left a nit about parametrizing the test, but other than that, looks good! Accept or reject my suggestion as you see fit.

@eccabay eccabay enabled auto-merge (squash) May 24, 2023 20:12
@eccabay eccabay merged commit c60f712 into main May 25, 2023
@eccabay eccabay deleted the 4187_recommendation_breakdown branch May 25, 2023 14:21
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.

Provide insight into recommendation score
3 participants