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

OWCollocations: widget for observing collocations #782

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

ajdapretnar
Copy link
Collaborator

@ajdapretnar ajdapretnar commented Mar 10, 2022

Issue

No widget for showing collocations.

Description of changes

OWCollocations widget.

Screenshot 2022-05-06 at 14 43 36

Includes
  • Code changes
  • Tests
  • Documentation

@ajdapretnar ajdapretnar marked this pull request as ready for review May 6, 2022 12:42
@ajdapretnar ajdapretnar changed the title Collocations OWCollocations: widget for observing collocations May 6, 2022
@ajdapretnar
Copy link
Collaborator Author

ajdapretnar commented May 6, 2022

This is now ready for a review and comments.
TODO:

  • It'd be nice if the widget showed colored bars, like Rank, but not sure how to do it. Thought I'd automatically get it by importing TableView from Rank.
  • Documentation.
  • Report.

@noahnovsak
Copy link
Contributor

noahnovsak commented Jun 3, 2022

Adding colored bars could be done by adding view.setItemDelegateForColumn(1, gui.ColoredBarItemDelegate()) in the widget's __init__, as TableRank overrides it for columns 0 and 1. Also self.collModel.setExtremesFrom(1, scores) would need to be called at some point after computing the scores.

If I understand what this widget does correctly, I imagine you could end up with quite a few rows here for larger datasets so running the computation concurrently makes sense. Also with radio buttons you can only view one set of scores at a time.
If these issues are not a concern then ignore this, otherwise it seems to me that some more functionality could easily be copied over from OWRank.

@ajdapretnar
Copy link
Collaborator Author

I imagine you could end up with quite a few rows here for larger datasets

Most likely. I'd leave concurrent implementation to masters. :)

Also with radio buttons you can only view one set of scores at a time.

This is intentional. The top words can vary significantly. I thought it makes more sense to view a single score at a time. Otherwise you'd have extremely long tables with many nans.

@ajdapretnar
Copy link
Collaborator Author

Also, thanks for the hints regarding the colored bars!

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2022

Codecov Report

Merging #782 (29eeb5c) into master (f371a63) will increase coverage by 0.03%.
The diff coverage is 85.84%.

@@            Coverage Diff             @@
##           master     #782      +/-   ##
==========================================
+ Coverage   76.80%   76.84%   +0.03%     
==========================================
  Files          84       85       +1     
  Lines       11957    12038      +81     
  Branches     1881     1886       +5     
==========================================
+ Hits         9184     9250      +66     
- Misses       2464     2476      +12     
- Partials      309      312       +3     

Copy link
Collaborator

@PrimozGodec PrimozGodec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks perfect. I just added a few minor comments.

orangecontrib/text/widgets/owcollocations.py Outdated Show resolved Hide resolved
orangecontrib/text/widgets/owcollocations.py Outdated Show resolved Hide resolved
orangecontrib/text/widgets/owcollocations.py Show resolved Hide resolved
@ajdapretnar
Copy link
Collaborator Author

@PrimozGodec As discussed, I've copied the code from Rank for now. Will submit another PR with changes from the master, which can be merged after we release Text and once Orange is released, too.

Other suggestions are also implemented.

@janezd
Copy link
Contributor

janezd commented Aug 22, 2022

If you really mean to someday remove the code from rank, you need to add a time bomb, that is, a test that will fail when Orange version is above XX, with a message to remove the code and this test.

@ajdapretnar
Copy link
Collaborator Author

You are absolutely right. I've added the test, hope it's ok.

@PrimozGodec PrimozGodec merged commit fd94fcb into biolab:master Aug 24, 2022
@PrimozGodec
Copy link
Collaborator

Nicely done. :)

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.

5 participants