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: Option to merge less frequent values #4477

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

PrimozGodec
Copy link
Contributor

@PrimozGodec PrimozGodec commented Mar 1, 2020

Issue

Fixes #4436

Description of changes

Added an option to merge less frequent values:

  • values that have less than n appearances
  • keep only n different values

The current implementation of merging selected values moved in a new dialogue.

Includes
  • Code changes
  • Tests
  • Documentation

@PrimozGodec PrimozGodec added the needs discussion Core developers need to discuss the issue label Mar 6, 2020
@PrimozGodec PrimozGodec force-pushed the domain-group-values branch from 26eafac to 776b432 Compare March 6, 2020 21:15
@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #4477 into master will increase coverage by 0.28%.
The diff coverage is 98.00%.

@@            Coverage Diff             @@
##           master    #4477      +/-   ##
==========================================
+ Coverage   83.16%   83.45%   +0.28%     
==========================================
  Files         268      276       +8     
  Lines       53990    55228    +1238     
==========================================
+ Hits        44903    46091    +1188     
- Misses       9087     9137      +50     

@PrimozGodec PrimozGodec force-pushed the domain-group-values branch 2 times, most recently from 6046713 to fde238d Compare March 9, 2020 11:53
@PrimozGodec PrimozGodec removed the needs discussion Core developers need to discuss the issue label Mar 9, 2020
@PrimozGodec PrimozGodec force-pushed the domain-group-values branch 2 times, most recently from 0726c24 to ec4b444 Compare March 9, 2020 13:38
@PrimozGodec PrimozGodec changed the title [WIP] Edit Domain: Merge less frequent values Edit Domain: Option to merge less frequent values Mar 9, 2020
@PrimozGodec PrimozGodec force-pushed the domain-group-values branch 2 times, most recently from 2545ae9 to 0a19883 Compare March 9, 2020 14:10
@lanzagar
Copy link
Contributor

After you select one of the merge options, set its parameter, apply the transformation and then see the result you might want to change the parameter. It is kind of annoying that opening the merge dialog again resets everything (it would be nicer to see what was selected previously and allow a minor correction).
Would that complicate the code a lot or is it simple to change?

@lanzagar
Copy link
Contributor

Also, don't forget to put an [ENH] tag in the PR title (for the changelog).

Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

This could be merged as is, but I have a list of small comments. Feel free to reject any or even all.

Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
@janezd
Copy link
Contributor

janezd commented Mar 13, 2020

It is kind of annoying that opening the merge dialog again resets everything

I also thought about this. Then I considered print dialogs, which usually do not remember page intervals, for instance.

But I agree with @lanzagar - if it's not too complicated to include this as a setting, please do.

@PrimozGodec PrimozGodec changed the title Edit Domain: Option to merge less frequent values [ENH] Edit Domain: Option to merge less frequent values Mar 13, 2020
@PrimozGodec PrimozGodec changed the title [ENH] Edit Domain: Option to merge less frequent values [WIP][ENH] Edit Domain: Option to merge less frequent values Mar 13, 2020
@PrimozGodec PrimozGodec force-pushed the domain-group-values branch 2 times, most recently from cbaaac9 to fb7f0fa Compare March 17, 2020 14:01
@PrimozGodec PrimozGodec changed the title [WIP][ENH] Edit Domain: Option to merge less frequent values [ENH] Edit Domain: Option to merge less frequent values Mar 17, 2020
@PrimozGodec PrimozGodec changed the title [ENH] Edit Domain: Option to merge less frequent values [WIP][ENH] Edit Domain: Option to merge less frequent values Mar 17, 2020
@PrimozGodec PrimozGodec force-pushed the domain-group-values branch from fb7f0fa to 86db948 Compare March 18, 2020 12:18
@PrimozGodec PrimozGodec changed the title [WIP][ENH] Edit Domain: Option to merge less frequent values [ENH] Edit Domain: Option to merge less frequent values Mar 18, 2020
@PrimozGodec
Copy link
Contributor Author

Now parameters are saved as a Context variable. The solution does not look nice, but I did not see any better option. If it looks too ugly we can remove it.

Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
@janezd janezd removed their assignment Mar 20, 2020
@PrimozGodec PrimozGodec force-pushed the domain-group-values branch from 86db948 to 60600f3 Compare March 20, 2020 11:37
@PrimozGodec PrimozGodec force-pushed the domain-group-values branch from 60600f3 to b5f546a Compare March 20, 2020 12:00
Copy link
Contributor

@lanzagar lanzagar left a comment

Choose a reason for hiding this comment

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

Apart from the extra sign in a docstring, the rest seems ok to me.

@lanzagar lanzagar merged commit 5022022 into biolab:master Mar 20, 2020
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.

An option to group less common values
3 participants