This repository has been archived by the owner on Nov 3, 2023. It is now read-only.
Add file for helper metrics for slots #3138
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Well, that's a bunch of automatically caught bugs. Time to put this back into draft mode... |
|
Putting this ready for review since logic seems good. Note that running the |
| # we drop Python 3.6 | ||
| if other is None: | ||
| return self | ||
| slot_p = _average_type_sum_helper(self._slot_p, other._slot_p) |
There was a problem hiding this comment.
We might if both arguments are Nones.
There was a problem hiding this comment.
Ya I just don't understand if/when that happens
There was a problem hiding this comment.
Ahhh will add a comment about this to clarify.
Basically if you look at SlotMetrics::__init__(), it's clever about how it aggregates F1 metric components, accumulating the precision and recall values separately. However, this means there might be Nones floating around. (I copied this behavior from the original implementation of SlotF1Metrics in the multiwoz agent.)
There was a problem hiding this comment.
@stephenroller this is what you had changed because you were occasionally getting Nones for the metric during validation, right?
|
I think autoformat assumes you're running from the top directory. That might be something to fix, but for now, just run from the root of the repo. |
…ltiple metrics be added to it See #3138 for context and use
moyapchen
force-pushed
the
slot_teachers
branch
from
76a5f54 to
d3b5a19
Compare
…g in python... (alas, that's what I get for mostly coding in C++ for the past few years. :P)
moyapchen
force-pushed
the
slot_teachers
branch
from
d3b5a19 to
b4479f5
Compare
moyapchen
force-pushed
the
slot_teachers
branch
from
c79a3a6 to
e3e35b7
Compare
moyapchen
added a commit
that referenced
this pull request
Oct 2, 2020
…3145) * Add notion of metrics collections, which can have other Metrics of multiple metrics be added to it See #3138 for context and use * right, having different arguments for the same function aren't a thing in python... (alas, that's what I get for mostly coding in C++ for the past few years. :P) * fixed a bug while integrating into taskmaster2 * address comments (get rid of separate class, add func to Metrics directly) * actually do the things the last comment
* Add test for interactive_web * Spinlock * Hm. * Lint.
* Allow missing init opt opts * Add part of unit test * Work on unit test * Test fixes * Fix second test * Fix test * Check obsolete arg does not exist
…ltiple metrics be added to it See #3138 for context and use
SlotF1Metric + SlotF1Metric test imported from an internal implementation. Will go through and refactor + test existing tasks to use this next.
Not quiet sure if returning "nans" for F1 metrics is the best way of going about things, but probably okay as a start. Also assuming the Metrics base class will handle addition correctly. Added the test case to make sure there weren't any glaring syntax errors with how I implemented SlotMetrics Nominally should probably add more test cases to validate slot-based domain metrics are working correctly, but that seems lower risk/lower pri and will cover it anyway when I update existing tasks.
added 8 commits
October 2, 2020 11:33
`docformatter -i --pre-summary-newline --wrap-descriptions 88 --wrap-summaries 88 --make-summary-multi-line` on the relevant file
…shing so all my mispellings don't go into the commit log...
Noticed while testing on taskmaster that jga was higher than both slot_r and slot_p... put in some prints and turns out it was counting the {} == {} case.
This doesn't really make sense to do in general, though there are some scnearios where not having any slots is the correct response... so put in a flag to compensate.
…addressing comments = adding comments + fixing grammar, mostly).
moyapchen
force-pushed
the
slot_teachers
branch
from
fea599b to
6b9a0a4
Compare
…3145) * Add notion of metrics collections, which can have other Metrics of multiple metrics be added to it See #3138 for context and use * right, having different arguments for the same function aren't a thing in python... (alas, that's what I get for mostly coding in C++ for the past few years. :P) * fixed a bug while integrating into taskmaster2 * address comments (get rid of separate class, add func to Metrics directly) * actually do the things the last comment
…ltiple metrics be added to it See #3138 for context and use
moyapchen
pushed a commit
that referenced
this pull request
Oct 2, 2020
…ltiple metrics be added to it See #3138 for context and use
moyapchen
added a commit
that referenced
this pull request
Oct 2, 2020
…3145) * Add notion of metrics collections, which can have other Metrics of multiple metrics be added to it See #3138 for context and use * right, having different arguments for the same function aren't a thing in python... (alas, that's what I get for mostly coding in C++ for the past few years. :P) * fixed a bug while integrating into taskmaster2 * address comments (get rid of separate class, add func to Metrics directly) * actually do the things the last comment
moyapchen
pushed a commit
that referenced
this pull request
Oct 2, 2020
…ltiple metrics be added to it See #3138 for context and use
moyapchen
added a commit
that referenced
this pull request
Oct 2, 2020
…3145) * Add notion of metrics collections, which can have other Metrics of multiple metrics be added to it See #3138 for context and use * right, having different arguments for the same function aren't a thing in python... (alas, that's what I get for mostly coding in C++ for the past few years. :P) * fixed a bug while integrating into taskmaster2 * address comments (get rid of separate class, add func to Metrics directly) * actually do the things the last comment
|
Converting this to draft mode, cause rebasing + git is doing weird stuff. |
moyapchen
pushed a commit
that referenced
this pull request
Oct 2, 2020
…ltiple metrics be added to it See #3138 for context and use
|
Abandoning this; will do internally and sync once it's all nice. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
SlotF1Metric + SlotF1Metric test imported from an internal implementation. Will go through and refactor + verify that I don't break existing tasks using this next.
Testing steps
`pytest tests/SlotF1Metric + SlotF1Metric test imported from an internal implementation. Will go through and refactor + verify that I don't break existing tasks using this next.
Testing steps
pytest tests/test_slot_metrics.pyNote that I did change the SlotF1Metric class (and its associated test) a bit in the 2nd change in this PR in order to get tests to run properly. (I got assertion errors trying to run the
pytest test_slot_f1_metrics.pyin its original internal directory so figured it would need to change anyhow.)