-
Notifications
You must be signed in to change notification settings - Fork 41
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
Issue290 upgrade fasttext #292
Conversation
Codecov Report
@@ Coverage Diff @@
## master #292 +/- ##
==========================================
+ Coverage 99.36% 99.39% +0.02%
==========================================
Files 55 55
Lines 2845 3130 +285
==========================================
+ Hits 2827 3111 +284
- Misses 18 19 +1
Continue to review full report at Codecov.
|
Thanks for the PR! It seems that the fastText tests are not running under Travis CI, which leads to low test coverage. I'm guessing that the reason is that in |
I fixed the installation in |
Thanks, much better now! The tests seem to be running fine after the change. The wiki page can simply be edited by anyone, including you. There is a bit of a synchronization challenge here - ideally the wiki would be changed at the same time the PR is merged. Perhaps we could simply drop the alternative recipe ( Do you think it would be easy to add a unit test for the pretrained vectors feature? It would require adding a file with some pretrained vectors and then adding a test (in |
I thought about making a test but dismissed the idea as I would have to add a huge file somehow, but a toy data test I can definitely do. |
There is still one problem that needs to be addressed before we can merge. With some test sets I get a ValueError:
This appears to be a bug in the upstream fastText module. The line |
Bug reported to fastText: facebookresearch/fastText#846 |
Seems the bug has now been fixed upstream, but we should probably wait until there's an official release of fastText with the bugfix included. |
Agree that we should wait for a PyPI release. |
fastText 0.9.2 has been released on PyPI: https://pypi.org/project/fasttext/ Meanwhile some merge conflicts have emerged, I will try to sort them out so that we can merge this for the next release 0.48. |
I couldn't figure out how to update this PR. I tried to push to the original PR branch on CSCfi/Annif but got a "permission denied" error, even though the PR seems to allow pushing by upstream users. Anyway, I created a new PR #409 instead. |
Upgrade fastText back to official PyPI version, and enable the pretrained vectors feature.
Closes #290