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

Polynomial Classification: Remove highcharts, other minor improvements #112

Merged
merged 11 commits into from
Jul 26, 2021

Conversation

janezd
Copy link
Collaborator

@janezd janezd commented Jul 18, 2021

Issue

Polynomial Classification uses highcharts. We'd prefer it didn't.

Doesn't pass tests on the oldest version of Orange, but should after merging #95, which increases the requirement.

Description of changes

Polynomial classification now uses the common OWScatterPlotBase.

Besides this,

  • a few minor changes in GUI -- rearrangements and hiding of unnecessary controls
  • removed option to set the contour step; 0.1 should be OK for everybody
  • refactoring the methods
Includes
  • Code changes
  • Tests
  • Documentation

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2021

Codecov Report

Merging #112 (c73f48e) into master (e7fb8b0) will decrease coverage by 1.01%.
The diff coverage is 85.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
- Coverage   84.93%   83.91%   -1.02%     
==========================================
  Files          20       20              
  Lines        3054     3097      +43     
  Branches      360      363       +3     
==========================================
+ Hits         2594     2599       +5     
- Misses        412      444      +32     
- Partials       48       54       +6     
Impacted Files Coverage Δ
.../educational/widgets/owpolynomialclassification.py 87.64% <85.81%> (-10.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7fb8b0...c73f48e. Read the comment docs.

@janezd janezd force-pushed the polyclass-no-highchart branch from 57324fd to 2035fa4 Compare July 22, 2021 07:57
@janezd janezd force-pushed the polyclass-no-highchart branch from 02c1a41 to bab6154 Compare July 23, 2021 09:30
@janezd janezd mentioned this pull request Jul 23, 2021
@PrimozGodec
Copy link
Collaborator

It looks great 👍 👍

I have a comment regarding colours. It seems colours (regions, dots, legend) does not match.

Here, for example, I would expect that Iris-setosa would be coloured with blue colour. The region seems to be coloured correctly and the legend for Iris-setosa is OK, just dots seem to have the wrong colour. Also, Iris-Virginica should be green but is blue now. I like the grey background for the non-selected classes.

Screenshot 2021-07-23 at 12 29 48

A similar problem appears when one select Iris-versicolor for a class.

@PrimozGodec
Copy link
Collaborator

PrimozGodec commented Jul 23, 2021

removed option to set the contour step; 0.1 should be OK for everybody

I think it was implemented that users can set it manually when they are too close together.

But the only effect of setting it to 0.2 is to remove some lines, or am I missing something? It does not make the plot more useful, I think. On the contrary, from dense lines one can see that the decision boundary is sharp.

I am also OK with merging this PR with the 0.1 settings and implementing this later if we decide on automation.

Yes, let's do so. If @BlazZupan later complains, we'll add automation (or I'll add a slider, the only sensible values are 0.1, 0.15 and 0.2, I suppose).

@janezd
Copy link
Collaborator Author

janezd commented Jul 24, 2021

I have a comment regarding colours. It seems colours (regions, dots, legend) does not match.

Oops. The idea was that the target class would be colored and other class(es) would be grayed out. With this, it is easier to decypher the target class and the meaning of probabilities -- if everyhing is colored, one must look at the combo to see the target. Obviously I haven't done it properly. Now I fixed it.

One minor annoyance here: the target class is always class 1, not 0, but this looks stupid in the legend, so I had to derive a class from OWScatterPlotBase which reverses the legend.

@PrimozGodec
Copy link
Collaborator

But the only effect of setting it to 0.2 is to remove some lines, or am I missing something? It does not make the plot more useful, I think. On the contrary, from dense lines one can see that the decision boundary is sharp.
Yes, let's do so. If @BlazZupan later complains, we'll add automation (or I'll add a slider, the only sensible values are 0.1, 0.15 and 0.2, I suppose).

You are right. I didn't think about the fact that those plots with sharp edges have the same probability elsewhere (0 or 1). Agree with leaving it 0.1

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.

3 participants