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

Word Enrichment: Computing in a separate thread (with ConcurrentMixin) #492

Merged
merged 2 commits into from
Feb 17, 2020

Conversation

PrimozGodec
Copy link
Collaborator

Issue

#221

Description of changes

Besides implementing the thread computation with ConcurrentWidgetMixin which fixes #221, this PR also:

  • implements input and output info in a status bar
  • adds better unit tests for a widget
  • refactors a code a bit
Includes
  • Code changes
  • Tests
  • Documentation

@PrimozGodec PrimozGodec force-pushed the enrichemnd-thread branch 2 times, most recently from 16cd80c to 1560eb7 Compare December 30, 2019 15:06
@codecov-io
Copy link

codecov-io commented Dec 30, 2019

Codecov Report

Merging #492 into master will decrease coverage by 0.59%.
The diff coverage is 87.71%.

@@            Coverage Diff            @@
##           master     #492     +/-   ##
=========================================
- Coverage   63.08%   62.48%   -0.6%     
=========================================
  Files          59       59             
  Lines        6279     6243     -36     
  Branches      824      814     -10     
=========================================
- Hits         3961     3901     -60     
- Misses       2180     2208     +28     
+ Partials      138      134      -4

@PrimozGodec PrimozGodec force-pushed the enrichemnd-thread branch 2 times, most recently from 3483ece to c5f8b21 Compare December 30, 2019 15:55
@ajdapretnar
Copy link
Collaborator

This a great PR. But I wouldn't be me if I didn't have any comments. 😬
Cluster words and Selected words is not clear to me. Cluster words apparently are all words and selected are non-zero ones (so those that actually exist in the subset). I think it would make more sense to say 'Total words' and 'Words in subset' or sth similar. Or perhaps get rid of Cluster words?

I am slightly confused by '18 words on the output'. The tooltip then explains that 18 words have been found, but do we have another widget that shows computational results instead of the output? I could be confusing for the users.

On the same note: tree (i.e. found words) should not be selectable. It can confuse the user, too.

@ajdapretnar
Copy link
Collaborator

Perhaps a solution for the status bar would be introducing a new icon that would replace the output icon (say a lightbulb) and signify the result of the widget.

@PrimozGodec
Copy link
Collaborator Author

I change texts and disabled the selection.

Perhaps a solution for the status bar would be introducing a new icon that would replace the output icon (say a lightbulb) and signify the result of the widget.

I will wait until we decide whether we implement this.

@PrimozGodec PrimozGodec changed the title Word Enrichment: Computing in a separate thread (with ConcurrentMixin) [WIP] Word Enrichment: Computing in a separate thread (with ConcurrentMixin) Jan 14, 2020
@PrimozGodec PrimozGodec changed the title [WIP] Word Enrichment: Computing in a separate thread (with ConcurrentMixin) Word Enrichment: Computing in a separate thread (with ConcurrentMixin) Feb 17, 2020
@PrimozGodec
Copy link
Collaborator Author

It should be finished now. @ajdapretnar can you check when you have time.

@ajdapretnar ajdapretnar merged commit 10139d7 into biolab:master Feb 17, 2020
@PrimozGodec PrimozGodec deleted the enrichemnd-thread branch March 29, 2023 10:38
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.

WordEnrichment Should Run in a Thread
3 participants