Skip to content
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

git rm gensim/models/flsamodel.py #3470

Merged
merged 1 commit into from
Aug 21, 2023
Merged

git rm gensim/models/flsamodel.py #3470

merged 1 commit into from
Aug 21, 2023

Conversation

mpenkov
Copy link
Collaborator

@mpenkov mpenkov commented May 10, 2023

No description provided.

@piskvorky
Copy link
Owner

piskvorky commented May 10, 2023

@mpenkov so we're axing the module? Either way, let me merge the FLSA PRs first.

In case someone wants to continue in the future, so that they have the "fixed" version, with the various improvements from the recent PRs by me and @ERijck.

@mpenkov
Copy link
Collaborator Author

mpenkov commented May 10, 2023

Yes, I think it's best to remove this. It's completely new code, and we don't have the resources to maintain it (pace of existing PRs related to this module is extremely slow).

Let me know when it's ready to torch.

@ERijck
Copy link
Contributor

ERijck commented May 10, 2023

That's unfortunate. But I understand your considerations. What needs to be changed so it meets the Gensim requirements later on? I understand that the main limitation now is the representation (strings instead of integer BOW); is that correct? Are you open to me fixing this issue and adding it to Gensim once ready?

Also, my Git understanding was lacking as this was my first project contributing to another repository. However, to solve this, I have followed the full Git course on Codecademy.

@piskvorky
Copy link
Owner

Are you open to me fixing this issue and adding it to Gensim once ready?

Yes, I am. The code is in much better shape already, after our recent PRs. If you can get its data representation in shape (same streaming input as the rest of Gensim, not everything in RAM) then I'll be happy to include FLSA.

For now, can you please fix #3437 (comment)? So we can merge that PR, it will be a starting point for any future revivals. Thanks!

@mpenkov mpenkov added this to the 4.3.2 milestone Aug 8, 2023
@mpenkov mpenkov merged commit bf19210 into develop Aug 21, 2023
@mpenkov mpenkov deleted the rmflsa branch August 21, 2023 15:19
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.

3 participants