Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Revised metrics proposal #15
base: master
Are you sure you want to change the base?
Revised metrics proposal #15
Changes from 22 commits
8c96695
531c195
66eab2f
cf8ae38
1d8987c
6d5c5e3
00e0a1a
5ce78ec
9061c70
5cd4e88
75e1d1d
aac662c
bf96d2b
416d427
fbb634f
77777ba
6d69fa9
13107f4
048e484
9ff4e91
5040649
210872e
3ba87aa
c13da58
13069d5
2e10c46
8296f3a
f755711
0f28fe3
2ea2037
41bb325
205c239
70d8811
96703c6
7521261
f9ed32c
091c47e
180370b
16983bf
acfacab
697eee0
f0cdb5c
4feb19f
5649f92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether we're already at the stage of discussing naming, but since we decided to discard
.eval()
, we might want to think about a name that better reflects that the class that's currently calledGroupedMetric
contains results/scores. The current name suggests that the class corresponds to some grouped metric itself rather than results/scores. Similarly, I'm not sure aboutgroup_summary()
, because in my viewGroupedMetric
is not really a summary? My suggestion would be to come up with a few alternatives and ask some people who are not as deeply involved with the metrics API so far which one they find most intuitive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great time to be discussing naming, I think..... I'll see if I can come up with a few possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very confusing to me. Defaults to relative_to="max"?
Why not let the user specify? Reads a lot better with the explicit specification as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean you'd like to have no default at all or a different one than "max"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user can specify, but I believe doing relative to
max
matches our current behaviour. I would be happy with not having a default too, if that was the consensus.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our current behavior is that we are taking differences relative to the minimum not maximum (that's why all of our differences are positive). That's how I wrote the proposal here. In other words, the "relative to" part is whatever you subtract from all the group-level metric values.
But maybe it's confusing? An alternative term could be "origin". Another idea would be to say "from" instead of "relative_to"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this different from the line above with no
abs
? The value was positive, so that should be identical, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without
abs
the values were -0.2436 and 0.1870. Withabs
the values are 0.2436 and 0.1870.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing, don't like the default at all :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the same from me: I'm happy to change if there's agreement that requiring the argument makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both this line and the next say
relative_to
, is that intended?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit worried about the complexity of the
ratio_order
argument, esp for new users.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The change is between numerator and denominator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a question of naming, or whether we need all the cases? I'm not overly happy with the names myself, but I do think that all four cases might be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of both, I guess... but I think you're right that you need all four cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can look at https://scikit-learn-enhancement-proposals.readthedocs.io/en/latest/slep006/proposal.html and scikit-learn/scikit-learn#16079 to get some ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading that correctly, we would ditch the
**kwargs
, and have an explicitprops=
dictionary - and probably renameindexed_params=
tosample_props=
. When evaluating multiple metrics at once, I still think we'd need a list of dictionaries and a list of lists for these, though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably would not include this in a first version, because it seems quite tricky. But I think we can add some sort of convenience function for this later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's going to be a thorny function to write (because it will need to be kept in sync with SciKit-Learn), and perhaps best deferred until later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously I'm just one data point, but I strongly prefer this over the version below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed - people were leaning that way, but since there's always the question of 'Do we really need a new concept?' I just wanted to call this out.