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

[MRG] Fixes against #2698 #10

Merged
merged 3 commits into from
Jul 10, 2020
Merged

[MRG] Fixes against #2698 #10

merged 3 commits into from
Jul 10, 2020

Conversation

piskvorky
Copy link

@piskvorky piskvorky commented Jul 6, 2020

Fixing some low-hanging fruit in @gojomo 's epic PR piskvorky#2698.

  • Resolve conflicts and merge develop.
    • Removed the recently added max_final_vocab; will need to look deeper what's going on there, following here.
  • Many code style fixes (hanging indents, .format etc).
  • Fix segment_wiki.py
    • This file had Windows line endings (??), my git refused to commit the file => converted to unix here.
  • Resolve comments in the original KeyedVectors & *2Vec API streamlining, consistency piskvorky/gensim#2698 that were already fixed by this PR.
    • Only to-be-resolved comments left.

I have a better sense of the changes and direction in piskvorky#2698 now. Several comments remain unresolved, but I find that PR impossible to work with on Github. It's so large I can't even track notifications – I have to manually expand all files and look for what changes, new comments… 😱. How do you guys do it?

So I'd propose:

  1. Fix critical issues that block merging here. I think there's just one but @mpenkov hasn't reviewed fully yet.
  2. Make sure tests pass (@mpenkov please see KeyedVectors & *2Vec API streamlining, consistency piskvorky/gensim#2698 (comment)).
  3. Merge this PR.
  4. Merge KeyedVectors & *2Vec API streamlining, consistency piskvorky/gensim#2698.
  5. Copy over unresolved questions, comments, TODO lists – somewhere where they're easier to track.
    • I personally use FIXME to denote sections that must be resolved before a release (like critical bugs, stubs) & grep for FIXME before a release, to verify none are left. And use TODO for things that can be released later, incrementally, more "wish list".
  6. Continue addressing TODOs in new (smaller, focused) PRs: renaming, refactoring, docs…
  7. Release 4.0

@piskvorky piskvorky changed the title Fixes against #2698 [MRG] Fixes against #2698 Jul 6, 2020
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