[ENH] Discretize: Simplify interface, add nicer binning #5919

janezd · 2022-03-27T16:46:12Z

Issue

Resolves #5867.

To do:

Description of changes

Includes

Code changes
Tests
Documentation

janezd · 2022-03-27T16:54:25Z

Hm, or even:

@markotoplak?

ajdapretnar · 2022-03-28T07:13:45Z

I don't like the fact that the default setting has a variable type icon, which is somehow renamed to D. I think it is misleading. There's should be no icon, imo.

I like the redesign though. Perhaps the second one is better?

janezd · 2022-03-30T16:17:29Z

@ajdapretnar, I agree that the icon is misleading. But without any icon, the setting looks like a weird label. I think this looks OK?

ajdapretnar · 2022-03-30T16:23:15Z

I like the star. I just would avoid visually similar icons to those we already have - so the star works perfectly.

janezd · 2022-03-30T16:23:57Z

Now for the fun part.

I load Iris data. I set the default to something, say MDL. [The context includes no attributes, therefore matches anything.]
I load Heart disease. Should default remain at MDL, because the settings I just had was matching? Or should it go back to default default?
I set some specific discretization for some variable, and I set default to Remove.
I change to Housing. Should default change to MDL, because the setting for Iris matches (on empty)? Or should it remain Remove, because that's what I have? Or should it reset to default default?
I change default to Binning with 2 bins.
I go back to Iris. Should default stay as it is, or change to MDL because that's what I've been doing with Iris?

These were simple questions. On Friday, we'll talk about handling different selections of variables from the same data set.

janezd · 2022-03-30T21:31:57Z

Does anybody know a decent way to migrate a non-context setting into a context setting?

Default method (which is applied to all variables without specific setting) used to be a non-context setting. Now it is a context setting - as it should be, and also the implementation of the widget is such that this would be difficult to change. Yet, Orange migrates non-context and context settings separately; the method for migrating one of them, doesn't see others.

markotoplak · 2022-03-31T04:50:36Z

@janezd, I think migrate settings sees contexts too (check the dict it is getting).

janezd · 2022-03-31T08:26:10Z

@janezd, I think migrate settings sees contexts too (check the dict it is getting).

Huh, thanks! I was looking at a wrong moment (in migration of default settings, which do not include contexts!) and concluded that they are somehow temporarily removed. I was already considering all kinds of hacks. Thanks!

Works like charm.

codecov · 2022-03-31T21:15:51Z

Codecov Report

Merging #5919 (4297c5a) into master (66e24de) will increase coverage by 0.02%.
The diff coverage is 97.06%.

@@            Coverage Diff             @@
##           master    #5919      +/-   ##
==========================================
+ Coverage   86.28%   86.31%   +0.02%     
==========================================
  Files         315      315              
  Lines       66899    67067     +168     
==========================================
+ Hits        57725    57890     +165     
- Misses       9174     9177       +3

janezd · 2022-04-01T20:15:58Z

@ajdapretnar, can I ask you to finish documentation? I have written the text (committed) and made screenshots (attached to this comment). Screenshots need to be stamped, saved in proper format and properly linked in the text. Thanks!

screenshots.zip

ajdapretnar · 2022-04-04T06:40:27Z

Will do!

lanzagar · 2022-04-08T10:33:36Z

Orange/widgets/data/owdiscretize.py

+        texts = set()
+        for key in varkeys:
+            dvar = self.discretized_vars.get(key)
+            fmt = self.data.domain[key[0]].repr_val


There are at least two ways this can crash:

self.data is None
AttributeError: 'NoneType' object has no attribute 'domain'
Reproduce: Add Discretize widget, open and click CC

key is DefaultKey/None (see L744)
TypeError: 'NoneType' object is not subscriptable
Reproduce: Connect some data to Discretize widget, click CC

86b10c0 should now fix it. The button should be disabled when the user selects "Default settings", and Default settings should be selected on "Set data" (which selects it when there is no data).

janezd · 2022-04-10T21:18:26Z

@ajdapretnar, I don't see it here, I'm asking just to be sure: have you by any chance already updated the documentation and I force pushed over it? If so -- I'm really sorry for this negligence. Please write down the hash of your commit, pull my changes and add your commit by cherry picking it. If you haven't, it's OK.

mw25 · 2022-04-11T08:13:35Z

I would like to add a question or a wish here:

Why is the number of bins for spinboxes limited to 10? With fixed width (100?) and custom (limited by the length of the input text?) the number is much higher.

I think 10 bins is a bit too little. At least in my cases.

ajdapretnar · 2022-04-12T06:50:03Z

@janezd Here it is: 621cf7a

janezd · 2022-04-19T19:49:36Z

@janezd Here it is: 621cf7a

@ajdapretnar can you backup these files, then pull my branch, cherry pick your commit and push? Again, backup these files first! (Otherwise, if you have those files, you can just recreate the commit instead of cherry picking it.)

@lanzagar, here's a puzzle for you. @ajdapretnar pushed this commit into my branch. I didn't pull it, but did some rebasing on my side and force-pushed, therefore removing her commit. I cannot cherry pick it, because I never got this commit and I don't know where to get it from -- fetching @ajdapretnar's fork doesn't do anything (it's not there, of course), nor can I get it from origin (it is not in any branch). Github, however, shows the commit, it just says it doesn't belong to any branch. Is there a way for me to get such a branchless, remoteless, forkless commit?

lanzagar · 2022-04-20T08:16:12Z

I added her commit to you branch and pushed to your repo, so I either
a) made a bigger mess
b) helped
Only time will tell...

janezd · 2022-04-20T15:35:43Z

I added her commit to you branch and pushed to your repo, so I either

How did you get her commit?

You fixed it -- thanks.

lanzagar · 2022-04-21T12:58:31Z

I don't know exactly what the trouble was for you, but I could cherry-pick the commit using the hash (after a standard pull from biolab's orange3 repo) so I just added it to you branch and pushed.
Maybe you never tried it, because you assumed you don't have the commit? The main orange3 repo stores some information from pull requests, so although @ajdapretnar pushed her commit to your repo (where you overwrote that branch) it was probably also floating around somewhere in biolab repository storage?

ajdapretnar · 2022-04-22T09:05:31Z

@janezd The macos failed tests seem not to be related to the PR (segfault and user warning not triggered on owfile). But ubuntu reports this, which does seem related:

ERROR: test_report_widgets_data (Orange.widgets.report.tests.test_report.TestReportWidgets)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/home/runner/work/orange3/orange3/.tox/orange-oldest/lib/python3.7/site-packages/Orange/widgets/report/tests/test_report.py", line 99, in test_report_widgets_data
   self._create_report(widgets, rep, data)
 File "/home/runner/work/orange3/orange3/.tox/orange-oldest/lib/python3.7/site-packages/Orange/widgets/report/tests/test_report.py", line 73, in _create_report
   w = self.create_widget(widget)
 File "/home/runner/work/orange3/orange3/.tox/orange-oldest/lib/python3.7/site-packages/orangewidget/tests/base.py", line 317, in create_widget
   widget.__init__()
 File "/home/runner/work/orange3/orange3/.tox/orange-oldest/lib/python3.7/site-packages/Orange/widgets/data/owdiscretize.py", line 525, in __init__
   self._create_buttons(box)
 File "/home/runner/work/orange3/orange3/.tox/orange-oldest/lib/python3.7/site-packages/Orange/widgets/data/owdiscretize.py", line 635, in _create_buttons
   self.button_group.idClicked.connect(self.update_hints)
AttributeError: 'QButtonGroup' object has no attribute 'idClicked'

Perhaps it's a dependency issue as the test says "Oldest dependencies"?

janezd added the needs discussion Core developers need to discuss the issue label Mar 30, 2022

janezd force-pushed the discretization-binning branch from c36cabe to 94c1bda Compare March 30, 2022 16:25

janezd force-pushed the discretization-binning branch 2 times, most recently from 2a7bad7 to ccbbe0a Compare March 31, 2022 20:57

janezd force-pushed the discretization-binning branch 5 times, most recently from c30e823 to a465ca4 Compare April 1, 2022 20:12

janezd marked this pull request as ready for review April 1, 2022 20:13

janezd removed the needs discussion Core developers need to discuss the issue label Apr 7, 2022

janezd assigned ajdapretnar Apr 8, 2022

lanzagar reviewed Apr 8, 2022

View reviewed changes

janezd force-pushed the discretization-binning branch from 621cf7a to 68fdce4 Compare April 10, 2022 20:47

janezd added 5 commits April 10, 2022 22:48

OWDiscretize: Reduce interface, add nicer binnings

b7d8703

OWDiscretize: Tidying up

0a80922

Discretize: Replace context settings with schema-only non-context

5efbf78

discretize: Use get_column_view instead of new table

23f0cb5

OWDiscretize: More tidying up

8e0a18f

OWDiscretize: Update documentation

4d61c1b

janezd force-pushed the discretization-binning branch from 68fdce4 to 4d61c1b Compare April 10, 2022 20:48

OWDiscretize: Disable copy-to-custom when default is selected

86b10c0

[DOC] Discretize: fixes

470d0b8

OWDiscretize: Add idClicked signal for PyQt < 5.15

4297c5a

ajdapretnar merged commit bc52372 into biolab:master Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Discretize: Simplify interface, add nicer binning #5919

[ENH] Discretize: Simplify interface, add nicer binning #5919

janezd commented Mar 27, 2022 •

edited

Loading

janezd commented Mar 27, 2022

ajdapretnar commented Mar 28, 2022

janezd commented Mar 30, 2022

ajdapretnar commented Mar 30, 2022

janezd commented Mar 30, 2022

janezd commented Mar 30, 2022

markotoplak commented Mar 31, 2022

janezd commented Mar 31, 2022

codecov bot commented Mar 31, 2022 •

edited

Loading

janezd commented Apr 1, 2022

ajdapretnar commented Apr 4, 2022

lanzagar Apr 8, 2022

janezd Apr 10, 2022

janezd commented Apr 10, 2022

mw25 commented Apr 11, 2022

ajdapretnar commented Apr 12, 2022

janezd commented Apr 19, 2022

lanzagar commented Apr 20, 2022

janezd commented Apr 20, 2022

lanzagar commented Apr 21, 2022

ajdapretnar commented Apr 22, 2022 •

edited

Loading

[ENH] Discretize: Simplify interface, add nicer binning #5919

[ENH] Discretize: Simplify interface, add nicer binning #5919

Conversation

janezd commented Mar 27, 2022 • edited Loading

Issue

Description of changes

Includes

janezd commented Mar 27, 2022

ajdapretnar commented Mar 28, 2022

janezd commented Mar 30, 2022

ajdapretnar commented Mar 30, 2022

janezd commented Mar 30, 2022

janezd commented Mar 30, 2022

markotoplak commented Mar 31, 2022

janezd commented Mar 31, 2022

codecov bot commented Mar 31, 2022 • edited Loading

Codecov Report

janezd commented Apr 1, 2022

ajdapretnar commented Apr 4, 2022

lanzagar Apr 8, 2022

Choose a reason for hiding this comment

janezd Apr 10, 2022

Choose a reason for hiding this comment

janezd commented Apr 10, 2022

mw25 commented Apr 11, 2022

ajdapretnar commented Apr 12, 2022

janezd commented Apr 19, 2022

lanzagar commented Apr 20, 2022

janezd commented Apr 20, 2022

lanzagar commented Apr 21, 2022

ajdapretnar commented Apr 22, 2022 • edited Loading

janezd commented Mar 27, 2022 •

edited

Loading

codecov bot commented Mar 31, 2022 •

edited

Loading

ajdapretnar commented Apr 22, 2022 •

edited

Loading