Make word_align_corpus train lazily - #344
Conversation
Enkidu93
left a comment
There was a problem hiding this comment.
This isn't used anywhere in silnlp, I don't believe. Is there anywhere else we should check / anyone else we should inform about this change in behavior?
@Enkidu93 reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
ddaspit
left a comment
There was a problem hiding this comment.
Transductive support was just added. I don't think anyone is using this function yet.
@ddaspit made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
Enkidu93
left a comment
There was a problem hiding this comment.
I'm probably missing something: Now when you call word_align_corpus(), the model will potentially be trained every time you call get_rows() on the returned corpus rather than once, right? Won't that affect callers who are using word_align_corpus() already?
@Enkidu93 made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
ddaspit
left a comment
There was a problem hiding this comment.
Yes, you are correct. It is a change in behavior, but it wasn't really "correct" before, because there was no way to clean up the model. I don't think any callers would be calling it multiple times anyway.
@ddaspit made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
Enkidu93
left a comment
There was a problem hiding this comment.
OK, right. I confirmed it isn't used like this in silnlp - just wanted to make sure there wasn't anything else we should check. But yes, a caller really shouldn't be calling get_rows() repeatedly anyways.
@Enkidu93 made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
- Properly cleans up the created model
de98a38 to
068b32e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
+ Coverage 91.94% 92.01% +0.07%
==========================================
Files 387 387
Lines 24381 24458 +77
==========================================
+ Hits 22416 22505 +89
+ Misses 1965 1953 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pmachapman
left a comment
There was a problem hiding this comment.
I don't think any callers would be calling it multiple times anyway.
This should be true for the dotnet implementation, as it is IEnumerable, and dotnet gives you a code analysis warning if you iterate over an IEnumerable multiple times. (I'm not aware of a similar feature in Python.)
@pmachapman reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ddaspit).
This change is