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

[FIX] SOM: Fix crash when color is constant #5860

Merged
merged 3 commits into from
Mar 14, 2022

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Feb 25, 2022

Issue

Fixes #5858.

If the variable determining the color was constant, the binning set two thresholds, which were both removed, thus creating an empty list of thresholds and labels. This created problems later in the code.

Description of changes

Set a threshold and label.

Includes
  • Code changes
  • Tests

@codecov
Copy link

codecov bot commented Feb 25, 2022

Codecov Report

Merging #5860 (c00df78) into master (269d7fa) will increase coverage by 0.00%.
The diff coverage is 97.29%.

@@           Coverage Diff           @@
##           master    #5860   +/-   ##
=======================================
  Coverage   86.29%   86.29%           
=======================================
  Files         315      315           
  Lines       66793    66815   +22     
=======================================
+ Hits        57638    57659   +21     
- Misses       9155     9156    +1     

@janezd janezd force-pushed the som-constant-color branch 2 times, most recently from c38d639 to a10f06a Compare February 25, 2022 11:00
@VesnaT VesnaT self-assigned this Feb 25, 2022
@VesnaT
Copy link
Contributor

VesnaT commented Feb 25, 2022

The fix works, except when the constant is NaN:

image

@janezd
Copy link
Contributor Author

janezd commented Feb 25, 2022

The fix works, except when the constant is NaN:

Mostly this. :) https://github.com/biolab/orange3/blob/master/Orange/widgets/unsupervised/owsom.py#L574

But also another place.

@@ -210,6 +211,8 @@ class Warning(OWWidget.Warning):
ignoring_disc_variables = Msg("SOM ignores categorical variables.")
missing_colors = \
Msg("Some data instances have undefined value of '{}'.")
no_defined_colors = \
Msg("'{}' has node defined values.")
Copy link
Contributor

@VesnaT VesnaT Feb 25, 2022

Choose a reason for hiding this comment

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

node -> no

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@VesnaT
Copy link
Contributor

VesnaT commented Mar 3, 2022

Similarly, the plot can't be colored by a Time Variable.

image

@janezd janezd self-assigned this Mar 4, 2022
@janezd
Copy link
Contributor Author

janezd commented Mar 9, 2022

Similarly, the plot can't be colored by a Time Variable.

This problem was actually in time_binning and would happen in any widget using this for discretization.

Which, by the way, reminds me: all widget should use time_binning/decimal_binning for discretization. E.g. Mosaic and Sieve, as well as all widgets that show color legends.

@VesnaT VesnaT merged commit fa64275 into biolab:master Mar 14, 2022
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.

SOM: IndexError when setting color with constant variable
2 participants