59 cosine hungarian

[jspaaks]

Originally here matchms/matchms-backup#246

So far we implemented a greedy version of the cosine score (which is often used because it is faster and gives the same results in 99,9% (that's a guess) of the actual cases.

The Hungarian algorithm would be the fully correct solution of the optimization problem, so it is nice to have it as well.

Here an example where the greedy scores would fail:

from matchms import Spectrum
from matchms.similarity import CosineGreedy, CosineGreedyNumba, CosineHungarian

test_spectrum1 = Spectrum(mz=np.array([100.005, 100.016]),
                         intensities=np.array([1.0, 0.9]),
                         metadata={})

test_spectrum2 = Spectrum(mz=np.array([100.005, 100.01]),
                         intensities=np.array([0.9, 1.0]),
                         metadata={})

similarity_measure = CosineGreedyNumba(tolerance=0.01)
print("CosineGreedy:", similarity_measure(test_spectrum1, test_spectrum2))

similarity_measure = CosineGreedy(tolerance=0.01)
print("CosineGreedyNumba:", similarity_measure(test_spectrum1, test_spectrum2))

similarity_measure = CosineHungarian(tolerance=0.01)
print("CosineHungarian:", similarity_measure(test_spectrum1, test_spectrum2))

CosineGreedy: (0.5524861878453039, 1)
CosineGreedyNumba: (0.5524861878453039, 1)
CosineHungarian: (0.994475138121547, 2)

[cwmeijer]

Nice addition. Good tests!

[cwmeijer]

Don't reuse variable name is posible. In this case you can just do:
return sorted(...)
This saves 1 line and you don't reuse the name. It's even better performance although I'm sure its negligible ;-)

[florian-huber]

Good point, thanks. Done.

[cwmeijer]

These first 5 lines can all move into the True branch of the if statement.

Also, quite a long function. It looks as if you could easily extract a function or 2 to reduce function size, variable in scope, and help the reader a bit with function names.
One obvious one would be:
def normalize_score(score, spec1, spec2):
return score/max(numpy.sum(spec1[:, 1]**2), numpy.sum(spec2[:, 1]**2))

Gets rid of 1 comment, helps readability a bit I'd say if you don't mind 1 liner functions (which I don't).

You could also extract the part that the comment refers to and call it 'solve_hungarian' or something like that.

Another option for function extraction I think would be everything in the True branch of 'if len(matching_pairs) > 0:'. The if statement would then read something like:
score, n_used_matches = calc_score_with_matches(matching_pairs) if len(matching_pairs) > 0 else (0,0)
Maybe that last option is a bit too complicated but I think it's ok. The line would be a bit hard to fit within 79 characters though which would damage readability a bit.

[florian-huber]

I tried to restructure the score calculation along your suggestions (I picked slightly different sub-functions though). Hope that makes it look better.

[cwmeijer]

Could you some how clearify how you got to the 'expected' values in all the tests? Did you calculate that manually or something like that? I'm just thinking what one would think if any of these tests start to fail at some point and you are not there to explain what you meant.
If the calculation is simple and clear, you may want to include it in the test as opposed to these 'magic' ground truth values.

[florian-huber]

I changed the test cases to simpler ones where I now compare the actual scores with expected values that are derived within the same test (same as for you similar comment in #38 ).

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

100.0% Coverage
0.0% Duplication

[fdiblen]

Refs #6

[florian-huber]

Thanks @cwmeijer for the review!