[FIX] Manifold Learning: Fix reseting of selected number of components #7191
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the manifold learning widget was resetting the user's selected number of components by constraining the spin control's maximum value. The fix introduces a new approach where users can set any value (up to 99), and a warning is displayed when the actual number of components is limited by the available data attributes.
Key Changes:
- Replaced dynamic spin control maximum constraint with a fixed upper limit of 99
- Added a new
act_componentsproperty that calculates the effective number of components based on data availability - Introduced a warning message to inform users when fewer components are created than requested
- Added comprehensive test coverage for the new component limitation behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Orange/widgets/unsupervised/owmanifoldlearning.py | Implemented act_components property, removed dynamic spin maximum constraint, added warning for component mismatch, and removed deprecated callbackOnReturn parameter |
| Orange/widgets/unsupervised/tests/test_owmanifoldlearning.py | Added test_too_few_attributes to verify warning behavior and updated test_n_components to use controls API |
| i18n/si/msgs.jaml | Added Slovenian translations for the new warning message and removed obsolete translation entry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.assertEqual(widget.act_components, 3) | ||
|
|
||
| spin.setValue(6) | ||
| assert widget.n_components == 6 |
There was a problem hiding this comment.
[nitpick] Using assert instead of self.assertEqual is inconsistent with the rest of the test file and unittest best practices. Consider using:
self.assertEqual(widget.n_components, 6)| assert widget.n_components == 6 | |
| self.assertEqual(widget.n_components, 6) |
There was a problem hiding this comment.
Use of assert here is OK.
I use assertEqual for things that I want to test (e.g. act_components) and assert for things that tests assume, before testing what they are intended to test.
| gui.spin( | ||
| output_box, self, "n_components", 1, 10, label="Components:", | ||
| controlWidth=QFontMetrics(self.font()).horizontalAdvance("0" * 10), | ||
| alignment=Qt.AlignRight, callbackOnReturn=True, | ||
| callback=self.settings_changed) | ||
| gui.rubber(self.n_components_spin.box) | ||
| alignment=Qt.AlignRight, | ||
| callback=self.settings_changed, | ||
| maximum=99) |
There was a problem hiding this comment.
The spin function is being called with conflicting parameters. The positional arguments 1, 10 specify minimum and maximum values, but then maximum=99 is also passed as a keyword argument. This creates ambiguity - the positional argument 10 will be overridden by maximum=99, but this is confusing.
Consider removing the positional 10 argument and only using keyword arguments for clarity:
gui.spin(
output_box, self, "n_components", 1, label="Components:",
controlWidth=QFontMetrics(self.font()).horizontalAdvance("0" * 10),
alignment=Qt.AlignRight,
callback=self.settings_changed,
minimum=1, maximum=99)or keeping the positional arguments and removing the keyword argument:
gui.spin(
output_box, self, "n_components", 1, 99, label="Components:",
controlWidth=QFontMetrics(self.font()).horizontalAdvance("0" * 10),
alignment=Qt.AlignRight,
callback=self.settings_changed)| self.send_signal(self.iris) | ||
| self.assertFalse(widget.Warning.less_components.is_shown()) | ||
| self.assertEqual(self.get_output().metas.shape[1], 3) | ||
| self.assertEqual(widget.act_components, 3) | ||
|
|
||
| self.send_signal(self.iris[:, :2]) | ||
| self.assertTrue(widget.Warning.less_components.is_shown()) | ||
| self.assertEqual(self.get_output().metas.shape[1], 2) | ||
| self.assertEqual(widget.act_components, 2) | ||
|
|
||
| self.send_signal(None) | ||
| self.assertFalse(widget.Warning.less_components.is_shown()) | ||
| self.assertIsNone(self.get_output()) | ||
|
|
||
| self.send_signal(self.iris[:, :2]) | ||
| self.assertTrue(widget.Warning.less_components.is_shown()) | ||
| self.assertEqual(self.get_output().metas.shape[1], 2) | ||
| self.assertEqual(widget.act_components, 2) | ||
|
|
||
| self.send_signal(self.iris) |
There was a problem hiding this comment.
The send_signal method is being called with only one argument self.iris, but elsewhere in the test file (e.g., line 32, 40) it's called with two arguments: the input signal and the data. This should be:
self.send_signal(widget.Inputs.data, self.iris)| self.send_signal(self.iris) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 3) | |
| self.assertEqual(widget.act_components, 3) | |
| self.send_signal(self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(None) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertIsNone(self.get_output()) | |
| self.send_signal(self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(self.iris) | |
| self.send_signal(self.widget.Inputs.data, self.iris) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 3) | |
| self.assertEqual(widget.act_components, 3) | |
| self.send_signal(self.widget.Inputs.data, self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(self.widget.Inputs.data, None) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertIsNone(self.get_output()) | |
| self.send_signal(self.widget.Inputs.data, self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(self.widget.Inputs.data, self.iris) |
There was a problem hiding this comment.
send_signal supports a single argument for widgets with only a single input. This is OK, in particular for widgets that are unlikely to have any additional inputs.
| self.assertEqual(self.get_output().metas.shape[1], 3) | ||
| self.assertEqual(widget.act_components, 3) | ||
|
|
||
| self.send_signal(self.iris[:, :2]) |
There was a problem hiding this comment.
The send_signal method is being called with only one argument, but elsewhere in the test file (e.g., line 32, 40) it's called with two arguments: the input signal and the data. This should be:
self.send_signal(widget.Inputs.data, self.iris[:, :2])| self.send_signal(self.iris[:, :2]) | |
| self.send_signal(self.widget.Inputs.data, self.iris[:, :2]) |
| self.assertEqual(self.get_output().metas.shape[1], 2) | ||
| self.assertEqual(widget.act_components, 2) | ||
|
|
||
| self.send_signal(None) |
There was a problem hiding this comment.
The send_signal method is being called with only one argument, but elsewhere in the test file (e.g., line 32, 40) it's called with two arguments: the input signal and the data. This should be:
self.send_signal(widget.Inputs.data, None)| self.send_signal(None) | |
| self.send_signal(widget.Inputs.data, None) |
| self.assertFalse(widget.Warning.less_components.is_shown()) | ||
| self.assertIsNone(self.get_output()) | ||
|
|
||
| self.send_signal(self.iris[:, :2]) |
There was a problem hiding this comment.
The send_signal method is being called with only one argument, but elsewhere in the test file (e.g., line 32, 40) it's called with two arguments: the input signal and the data. This should be:
self.send_signal(widget.Inputs.data, self.iris[:, :2])| self.send_signal(self.iris[:, :2]) | |
| self.send_signal(widget.Inputs.data, self.iris[:, :2]) |
| self.send_signal(self.iris) | ||
| self.assertFalse(widget.Warning.less_components.is_shown()) | ||
| self.assertEqual(self.get_output().metas.shape[1], 3) | ||
| self.assertEqual(widget.act_components, 3) | ||
|
|
||
| self.send_signal(self.iris[:, :2]) | ||
| self.assertTrue(widget.Warning.less_components.is_shown()) | ||
| self.assertEqual(self.get_output().metas.shape[1], 2) | ||
| self.assertEqual(widget.act_components, 2) | ||
|
|
||
| self.send_signal(None) | ||
| self.assertFalse(widget.Warning.less_components.is_shown()) | ||
| self.assertIsNone(self.get_output()) | ||
|
|
||
| self.send_signal(self.iris[:, :2]) | ||
| self.assertTrue(widget.Warning.less_components.is_shown()) | ||
| self.assertEqual(self.get_output().metas.shape[1], 2) | ||
| self.assertEqual(widget.act_components, 2) | ||
|
|
||
| self.send_signal(self.iris) |
There was a problem hiding this comment.
The send_signal method is being called with only one argument, but elsewhere in the test file (e.g., line 32, 40) it's called with two arguments: the input signal and the data. This should be:
self.send_signal(widget.Inputs.data, self.iris)| self.send_signal(self.iris) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 3) | |
| self.assertEqual(widget.act_components, 3) | |
| self.send_signal(self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(None) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertIsNone(self.get_output()) | |
| self.send_signal(self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(self.iris) | |
| self.send_signal(widget.Inputs.data, self.iris) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 3) | |
| self.assertEqual(widget.act_components, 3) | |
| self.send_signal(widget.Inputs.data, self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(widget.Inputs.data, None) | |
| self.assertFalse(widget.Warning.less_components.is_shown()) | |
| self.assertIsNone(self.get_output()) | |
| self.send_signal(widget.Inputs.data, self.iris[:, :2]) | |
| self.assertTrue(widget.Warning.less_components.is_shown()) | |
| self.assertEqual(self.get_output().metas.shape[1], 2) | |
| self.assertEqual(widget.act_components, 2) | |
| self.send_signal(widget.Inputs.data, self.iris) |
| " * (n_components + 3) / 2]": | ||
| n = self.n_components * (self.n_components + 3) / 2 | ||
| self.Error.n_neighbors_too_small("{}".format(n)) | ||
| n = self.act_components * (self.act_components + 3) / 2 |
There was a problem hiding this comment.
The calculation self.act_components * (self.act_components + 3) / 2 uses floating-point division and may result in a float value (e.g., 7.5), but the error message expects a number of neighbors which should be an integer. Consider using integer division // or explicitly converting to int:
n = self.act_components * (self.act_components + 3) // 2| n = self.act_components * (self.act_components + 3) / 2 | |
| n = self.act_components * (self.act_components + 3) // 2 |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (85.71%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #7191 +/- ##
=======================================
Coverage 88.92% 88.92%
=======================================
Files 335 335
Lines 74191 74195 +4
=======================================
+ Hits 65972 65981 +9
+ Misses 8219 8214 -5 🚀 New features to boost your workflow:
|
markotoplak
changed the title
janezd
force-pushed
the
manifold-less-components
branch
from
0093685 to
a229ce0
Compare
2 participants
Issue
Fixes #7163 and a few deprecation warnings.
Description of changes
The original sin was that the limit on the number of components was enforced by setting the maximal value of the spin, thus changing the user's setting. I think a better approach is to let users set whatever they want and show a warning if the actual number of components (which is limited by the data) is lower.
Further considerations:
Includes