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

Make model selector metadata to metric more robust #386

Closed
wants to merge 3 commits into from

Conversation

erica-chiu
Copy link
Contributor

@erica-chiu erica-chiu commented Aug 20, 2019

Related issues

Describe the proposed solution
Change the matching when parsing model selector metadata into metrics from name of metric to metric keys

Describe alternatives you've considered
N/A

Additional context
Allows for parsing of metadata to be based on content rather than the name, allowing for multiple metrics of the same type

@codecov
Copy link

codecov bot commented Aug 20, 2019

Codecov Report

Merging #386 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
+ Coverage   86.85%   86.86%   +<.01%     
==========================================
  Files         336      336              
  Lines       10950    10956       +6     
  Branches      351      578     +227     
==========================================
+ Hits         9511     9517       +6     
  Misses       1439     1439
Impacted Files Coverage Δ
...op/stages/impl/selector/ModelSelectorSummary.scala 93% <100%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a4a73...79c9083. Read the comment docs.

@erica-chiu erica-chiu removed their assignment Aug 20, 2019
nm -> JsonUtils.fromString[MultiClassificationMetrics](valsJson).get
case OpEvaluatorNames.Regression.humanFriendlyName =>
case `regression` =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, it is better not to have this hard coded as text, but I noticed a problem with matching in Enumns in my PR as well... @tovbinm is this related to the upgrade?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird. Looking at it...

@tovbinm
Copy link
Collaborator

tovbinm commented Aug 20, 2019

@erica-chiu proposed tests are defining the custom evaluator incorrectly. Here is how is supposed to work #387

@tovbinm tovbinm closed this Aug 20, 2019
@tovbinm tovbinm deleted the ec/modelInsightMetrics branch August 20, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants