Rename SpanPruner -> Pruner, remove -infs #1703
Conversation
allennlp/modules/__init__.py
Outdated
| from allennlp.modules.pruner import Pruner | ||
| # TODO(mattg): This line is to keep backwards compatibility. I'm not sure how to give a | ||
| # deprecation warning on using this import, but we should remove this around version 0.8. | ||
| from allennlp.modules.pruner import Pruner as SpanPruner # pylint: disable=reimported |
There was a problem hiding this comment.
This doesn't actually preserve backward compatibility, e.g what if someone had from allennlp.modules.span_pruner import .....
There was a problem hiding this comment.
Maybe this isn't a huge deal though, up to you.
There was a problem hiding this comment.
Yeah, I'm not sure there's a way to fix that. I'm hoping that most people will have imported from the base module, which is what we typically do. @joelgrus, do you know of a better way we can handle backwards compatibility with renaming modules like this?
There was a problem hiding this comment.
I think you can just leave the span_pruner.py there and import the Pruner as SpanPruner?
There was a problem hiding this comment.
That would also be a natural place for the depreciation warning.
There was a problem hiding this comment.
Then I have to have docs for it, because of our documentation check... Ugh. Ok.
There was a problem hiding this comment.
just add it to the docs to ignore dictionary
There was a problem hiding this comment.
Thanks, I had forgotten about that.
|
@DeNeutoy, I thought that test failure was a fluke, as it's not related to anything changed in this PR. But it's failed three times in a row now. Any ideas about what's going on there? |
|
No idea... it's quite high precision, does it go away if you drop it to 6? |
* Rename SpanPruner -> Pruner, remove -infs * Pylint and docs * Actually fix docs... * Make the deprecation warning better, better preserve compatibility, try to fix flaky test * Remove unused import
Fixes #1696. I decided against modifying the coref model in the places that it uses the log mask, because that was more complicated than I wanted to get to, and the coref model's loss function is able to handle the
-infswithout a problem.Git is saying that I removed
span_pruner.pyand addedpruner.py; that's just because I made cosmetic changes to a ton of lines, removing references to "spans". The only change in logic is usingreplace_masked_valuesinstead ofmask.log().