-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
[ENH] Document embedding #504
Conversation
d4b7b39
to
ccaf963
Compare
ccaf963
to
d90a5d1
Compare
Codecov Report
@@ Coverage Diff @@
## master #504 +/- ##
==========================================
+ Coverage 63.76% 64.91% +1.15%
==========================================
Files 59 62 +3
Lines 6306 6539 +233
Branches 828 851 +23
==========================================
+ Hits 4021 4245 +224
- Misses 2151 2157 +6
- Partials 134 137 +3 |
d90a5d1
to
a97532b
Compare
a97532b
to
14fd60d
Compare
f5d2ea5
to
41c11a4
Compare
a48d225
to
c775a4a
Compare
a73d351
to
9f2919b
Compare
I like this widget, it works well. I think it we should merge this as soon as it is remotely operational and then we address some minor details in future PRs. Something that could probably be improved is what happens when the internet is off. It works quite well in Image Embedding, but here it still takes a while for the error to appear. Perhaps it can't be fixed, perhaps it can. Also, why is auto_commit box so big? There's a weird extra space above the button, but not because of the PR - it seems to be the problem of the inherited box itself. Any ideas how to fix this? |
9f2919b
to
9f319a3
Compare
@ajdapretnar |
07261b6
to
256de32
Compare
When the internet is off there are two possible cases:
The last case is less probable. The same error is in the Image embedding. I think this behavior is ok from the user's point of view. Also when the internet is off and you request a specific website in the browser it will try to get it for you for some time and then write it is not reachable. |
@ajda for me widget do not have any extra space around the auto-apply box |
1ceea31
to
01c92de
Compare
Now I see what @ajdapretnar taught with the extra space. It seems that it appears only at Ubuntu (maybe Windows). @djukicn, do you also have this extra space above the auto-commit button on the other widgets? Now URL is set and Matjaz said that the server can be in production for the document embedder. So from my side, it can be merged. @ajda I suggest that you merge it if everything is ok from your side. |
Perfect, I will have a final look and merge. I can't wait for this one! 🎉 |
01c92de
to
1faaeb9
Compare
@PrimozGodec @ajdapretnar I've managed to get rid of the empty space by adding |
@djukicn could you please rebase the branch so that I can merge asap? :) Thanks! |
1faaeb9
to
6e9cee7
Compare
@ajdapretnar Done. |
Issue
Bag of words is currently the only method for vector representation of documents in text add-on. This pull request adds another way for doing it using fastText pretrained models described in:
E. Grave, P. Bojanowski, P. Gupta, A. Joulin, T. Mikolov,
Learning Word Vectors for 157 Languages.
Proceedings of the International Conference on Language Resources and Evaluation, 2018.
Description of changes
How to test?
Until the cluster does not get a permanent address is not hardcoded in the script. To test run Orange with setting the environment variable:
The environment variable can be also added in the configuration in PyCharm.
Includes