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

[ENH] Edit Domain: Merge categorical values #3725

Merged
merged 8 commits into from
Apr 12, 2019

Conversation

ales-erjavec
Copy link
Contributor

Issue

Implements gh-3371

Also fixes and assertion error when two values are renamed to the same value.

Traceback (most recent call last):
  File "/Applications/Orange3.app/Contents/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/Orange/widgets/data/oweditdomain.py", line 1165, in commit
    var = apply_transform(v, tr)
  File "/Applications/Orange3.app/Contents/Frameworks/Python.framework/Versions/3.6/lib/python3.6/functools.py", line 803, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
  File "/Applications/Orange3.app/Contents/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/Orange/widgets/data/oweditdomain.py", line 1430, in apply_transform_discete
    dest_codes = positions(dest_values)
  File "/Applications/Orange3.app/Contents/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/Orange/widgets/data/oweditdomain.py", line 1427, in positions
    assert len(rval) == len(values)
AssertionError
Description of changes

Implement merging of categorical values.

Merging can be achieved by simply renaming individual values to the same name, or by selecting multiple values and pressing the merge button (shortcut ctrl + =) and entering/selecting a name in an editable combo box.

Includes
  • Code changes
  • Tests
  • Documentation

@ales-erjavec ales-erjavec changed the title [ENH] Edit Domain: Merge categorical features [ENH] Edit Domain: Merge categorical values Apr 5, 2019
@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #3725 into master will increase coverage by 0.27%.
The diff coverage is 96.48%.

@@            Coverage Diff             @@
##           master    #3725      +/-   ##
==========================================
+ Coverage   84.53%   84.81%   +0.27%     
==========================================
  Files         373      374       +1     
  Lines       68471    68749     +278     
==========================================
+ Hits        57884    58307     +423     
+ Misses      10587    10442     -145


import numpy as np

import Orange.data

from Orange.preprocess.transformation import Identity, Lookup
from Orange.widgets import widget, gui, settings
from Orange.widgets.data.owconcatenate import unique
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid importing functions from widget modules if possible because it messes up dependencies and complicates future refactorings. I think we'd better treat all functions in widget modules as private (even if not named as such), just as we treat widget classes as final.

Do we have a better place for functions like unique?

Perhaps better, isn't unique(s) same as iter(dict.fromkeys(s)) (formally from Python 3.7, de facto already in 3.6). In below code, it's used as list(unique(s)), which is, I suppose, the same as list(dict.fromkeys(s)).

@janezd janezd self-assigned this Apr 12, 2019
@janezd janezd merged commit 761dd15 into biolab:master Apr 12, 2019
@ales-erjavec ales-erjavec deleted the oweditdomain-merge branch May 20, 2019 11:31
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