[ENH] kNN with the Cosine metric

[gmolledaj]

Issue

Implements the proposed solution discussed in #7270.

Description of changes

This PR adds an optional "Normalize instances (L2)" setting to the kNN widget.

When enabled, each input instance is normalized to unit L2 norm before learning by adding an internal preprocessor to the learner pipeline while preserving any existing preprocessors.
When enabled, the learner first runs Orange’s regular preprocessing pipeline and then applies row-wise L2 normalization before fitting Euclidean kNN.

The option is available only when the Euclidean metric is selected, since L2-normalized Euclidean distance provides the same nearest-neighbor ranking as cosine similarity for embeddings. This makes the widget more suitable for text embedding workflows without requiring a Python Script widget or globally modifying the preprocessing pipeline.

Includes

• Code changes
• Tests
• Documentation

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.98%. Comparing base (86f208c) to head (e0e4ae9).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7309      +/-   ##
==========================================
- Coverage   88.98%   88.98%   -0.01%     
==========================================
  Files         336      336              
  Lines       74544    74552       +8     
==========================================
+ Hits        66332    66338       +6     
- Misses       8212     8214       +2     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[janezd]

@gmolledaj, thanks for a well-made PR. I have a few comments.

1. Learners have default preprocessors -- in case of KNN (and most others models based on SKL fitters), they

   • exclude instances with undefined classes,
   • replace discrete variables with dummy variables,
   • remove variables without any defined values and
   • impute remaining missing values.

   These preprocessors are applied only if there are no user-defined preprocessors, i.e. preprocessors given as an input to the widget. In case there are, it's the user's responsibility to handle any categorical or missing data (or ensure there is None). The solution in this PR adds a preprocessor to the list that is meant for user-defined preprocessors. While this is a smart idea, it will prevent the application of default preprocessors listed above. The current PR will fail for this data set if normalization is checked: data.xlsx.zip

   My knowledge of Orange's classification and regression models are limited (I wrote the first implementation and the current implementation is attempt by other to clean my mess), but I think the simplest implementation that would follow this implementation would be to override the method preprocess of Orange.regression.knn.KNNRegressionLearner and Orange.classification.knn.KNNLearner.

   Let @markotoplak and @VesnaT tell whether there are any more appropriate methods to override instead.

   The SKL model also allows passing a callable as metric, but this would (I'm guessing here) prevent the model to use advanced methods to search for neighbours (like various trees), so preprocessing is probably a better approach.

2. How should this normalization treat categorical variables? Default preprocessing replaces it with dummy indicator variables; I suppose we shouldn't touch them? In this case, the preprocessing from the previous point should be applied before calling super().preprocess() and only preprocess numeric variables.

3. The widget also offers Manhattan distance. Would it make sense to offer (L1?) normalization for it?

4. Regarding the GUI: Distance (and perhaps some other widgets) offer "Euclidean" and "Euclidean (normalized)" (as well as Manhattan and Manhattan (normalized)) as two separate options. We could follow this pattern here, too. It needs clicking and, in particular, no code required to enable and disable the checkbox.

   The downside is that this would probably reorders the options in the combo, which would change the indices and invalidate the current workflows. I volunteer to write the migration.

   If we go this route, combo can be replaced with radio buttons. It is more consistent with Distances widget and the user sees what is available, on the other hand most classification/regression widgets prefer combos. I don't have a strong preference here.

[gmolledaj]

Thank you very much for the detailed review and for identifying the interaction with the learner's default preprocessors. I had not realized that adding the L2 normalizer to the user-defined preprocessor sequence would suppress the default preprocessing pipeline. I agree that this must be corrected.

Overriding the learner's preprocess method seems like a better integration point, although I would prefer to wait for @markotoplak and @VesnaT to confirm the most appropriate hook before changing the implementation.

Regarding categorical variables, my original use case consists of document embeddings, so the input is normally continuous. More generally, I agree that dummy indicator variables should probably not be affected by instance normalization. The exact ordering is important, however, because normalization must also coexist correctly with missing-value imputation. I would be happy to implement whichever ordering the maintainers consider consistent with Orange's preprocessing architecture.

Concerning Manhattan distance, my original proposal was specifically motivated by the exact neighbour-ranking equivalence between cosine distance and Euclidean distance on L2-normalized vectors. L1 normalization combined with Manhattan distance does not provide the same cosine-equivalence argument, so I would prefer not to add it merely for symmetry unless there is a separate use case for it.

Presenting this as separate options such as Euclidean and Euclidean (normalized) also sounds reasonable and would simplify the GUI logic. Before following that pattern, I would only like to confirm that “normalized” in the Distances widget refers to the same per-instance normalization rather than feature-wise normalization. If it does, consistency with that widget would be a strong argument in favour of this design.

Thank you also for offering to write the settings migration. Once there is agreement on the preprocessing hook and GUI design, I will be glad to revise the PR accordingly.

[janezd]

I'm sorry, my bad. I have read that your intention was to implement cosine distance, but I forgot it and reverted back to thinking about feature-wise normalization. Thanks for your patience. :)

This shows that @markotoplak is right: if this is a cosine distance, why not call it such? Computing Euclidean distance on normalized rows (rather than passing metric="cosine" to SKL) is just an implementational detail. If a user wants cosine distance, (s)he will look for it under that name, especially if working with texts.

I need to revisit my points.

1. Still applies.
2. I think that the proper approach would be to show an error if data includes categorical features. The assumption of cosine distance is that features are "homogenous" in the sense they measure "same things", like word frequencies. If some features are categorical, then - I think - cosine distance doesn't apply.
3. L1 of course no longer makes sense. (But this, I think, supports @markotoplak 's case that we should use the term cosine, not L2).
4. If we call it cosine, it belongs to the combo.

[gmolledaj]

Thank you for the clarification, Janez.

I completely agree that, from a user's perspective, someone working with text embeddings will naturally look for "cosine" rather than "L2 normalization". If the consensus is that the GUI should expose it as "Cosine", I have no objection to that.

I only wanted to clarify one technical point. My implementation does not introduce a cosine metric into the kNN learner. Instead, it adds an optional row-wise L2 normalization step before applying the existing Euclidean metric. The motivation comes from the mathematical equivalence that, for unit-normalized embeddings, Euclidean kNN yields the same neighbour ranking as cosine similarity. In that sense, I see it as a cosine-equivalent approach rather than a cosine implementation.

Regarding the preprocessing pipeline, thank you for pointing out the issue. I now have a revised implementation that no longer injects the L2 normalizer into the user preprocessors. Instead, it preserves both the learner's default preprocessing pipeline and any external preprocessors connected to the widget, and applies the optional L2 normalization afterwards before fitting the Euclidean kNN. I'll update the PR with this approach.

I also agree that the handling of categorical variables should be discussed separately. My original use case is document embeddings, where all features are continuous, but I'm happy to follow whichever behaviour is considered most appropriate for Orange.

[gmolledaj]

As a quick sanity check, I also tested the current implementation in a real Orange Text workflow:

Corpus → Document Embedding → kNN → Test & Score

The screenshot below compares the standard Euclidean kNN with the proposed optional L2 normalization enabled. Both workflows execute correctly, and the L2-normalized version obtains slightly better results on this corpus.

[gmolledaj]

Updated the PR following the review comments.

The L2 normalization is no longer added to the user-defined preprocessor list. The classification and regression learners now override preprocess(), first preserving Orange’s standard preprocessing pipeline and any externally supplied preprocessors, and then applying row-wise L2 normalization.

I also added tests covering:

· dense and sparse data,
· zero vectors,
· preservation of the input data,
· coexistence with user preprocessors,
· missing and categorical data processed through the default pipeline.

I rebased the branch onto the current master and ran:

pytest Orange/widgets/model/tests/test_owknn.py

Result: 29 passed, 3 skipped.

The UI is still exposed as the Euclidean metric with an optional “Normalize instances (L2; cosine-equivalent)” checkbox. I left the possible change to a dedicated “Cosine” entry in the metric combo for maintainers to decide.

[gmolledaj]

Thanks! I pulled the latest changes, tested the new implementation locally (including on an embeddings workflow), and everything looks good to me.

[janezd]

@gmolledaj , reading your code and thinking about removing the checkbox, I stumbled upon an alternative solution, which I pushed as another commit to this PR; we can remove it if we find it's not what we want. The best way to review it is to compare it with master, not as a separate commit, because it a lot of code from your previous commits.

The idea is to not use the preprocessor mechanism at all, but just normalize within a fit method and pass "euclidean" as an argument to scikit.

With this, the changes in the widget are minimal (and require no tests), and other changes are much simpler to implement (and test), too.

Please see if this works as you intended. @markotoplak or @VesnaT would also need to check whether I have correctly overridden the wrapper.

@VesnaT, I have put the code that manipulated the learner's params into _initialize_wrapped to avoid change the learner's parameters when fitting -- the same learner might be shared by multiple threads. Is your code (from 2016, I'm sure you still remember it :) that manipulates metric_params safe for multithreading?

[janezd]

@gmolledaj , thanks, I was writing the previous message before seeing you already reviewed the changes. Now we must wait for @markotoplak or @VesnaT to review the PR.

[markotoplak]

To me this PR seems fine.

The existing code, where fit() is editing self.params is indeed suspicious.