Skip to content

Make word_align_corpus train lazily - #344

Open
ddaspit wants to merge 1 commit into
mainfrom
lazy-word-align-corpus
Open

Make word_align_corpus train lazily#344
ddaspit wants to merge 1 commit into
mainfrom
lazy-word-align-corpus

Conversation

@ddaspit

@ddaspit ddaspit commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
  • Properly cleans up the created model

This change is Reviewable

- Properly cleans up the created model

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:lgtm:

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Transductive support was just added. I don't think anyone is using this function yet.

@ddaspit made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on pmachapman).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants