Fix 128bit SimHash, and add 256bit and 512bit SimHash #5

nicolaichuk · 2017-09-14T18:47:41Z

@tgalopin, hi.
Please review and merge to upstream.

add 256 and 512 bit SimHash

tgalopin

Thanks for your PR!

I'm not sure what you want to fix in 128 bits version and why you need all the modifications you did. Unfortunately, I can't accept such a PR without some explanations about your work and fixes on the coding style and BC breaks.

tgalopin · 2017-09-14T19:14:08Z

lib/Tga/SimHash/Comparator/GaussianComparator.php

+     * @param Fingerprint $fp1
+     * @param Fingerprint $fp2
+     * @return int
+     */


I'm pretty sure this method is slower than the current one, why do you need this?

Got it, thanks :) .

You can't use scalar type hint, we support php 5.3+

remove scalar type hint

tgalopin · 2017-09-14T19:14:42Z

lib/Tga/SimHash/Comparator/GaussianComparator.php

        $this->deviation = $deviation;
    }

+    /**


Count differences between fingerprints

tgalopin · 2017-09-14T19:16:19Z

lib/Tga/SimHash/Fingerprint.php

     * @var float
     */
-    protected $decimalValue;
+    protected $value;


This is a BC break without any added value, could you revert this?

"decimalValue" save value as number.
"value" save value as string.

These are two different variables, so they are called differently.

Could you make this property private as well? I don't think it should be exposed anyway.

tgalopin · 2017-09-14T19:17:10Z

lib/Tga/SimHash/Tokenizer/String128Tokenizer.php

    public function tokenize($element)
    {
-        return str_pad(base_convert(md5($element), 16, 2), 128, '0', STR_PAD_LEFT);
+        $hash = md5($element);


Why do you need to split this in parts?

For more comfort debugging

tgalopin · 2017-09-14T19:17:44Z

lib/Tga/SimHash/Tokenizer/String128Tokenizer.php

 class String128Tokenizer implements TokenizerInterface
 {
+
+    protected static $search = array('0','1','2','3','4','5','6','7','8','9','a','b','c','d','e','f');


You shouldn't add protected variables like this without a specific need as they will need to be handled by BC promise.

tgalopin · 2017-09-14T19:18:08Z

lib/Tga/SimHash/Tokenizer/String256Tokenizer.php

+    {
+        return $size === 256;
+    }
+}


Please add an ending line to this file.

tgalopin · 2017-09-14T19:18:14Z

lib/Tga/SimHash/Tokenizer/String512Tokenizer.php

+    {
+        return $size === 512;
+    }
+}


Please add an ending line to this file.

* add an ending line to this file

nicolaichuk · 2017-09-19T20:06:57Z

@tgalopin

I'm not sure what you want to fix in 128 bits version and why you need all the modifications you did.

first:
"base_convert" in 128 bits version always return string content only zero symbols.
(environment: php 7.1, windows 7).

second:
php "bindec" dont corect work with 128-bit value
how do you try to save 128-bit value in 32-bit float?

tgalopin

Thanks for your work, I understand it now :) . A few comments and we should be okay :) !

tgalopin · 2017-09-19T20:14:34Z

lib/Tga/SimHash/Comparator/GaussianComparator.php

+     * @param Fingerprint $fp1
+     * @param Fingerprint $fp2
+     * @return int
+     */


Got it, thanks :) .

tgalopin · 2017-09-19T20:17:44Z

lib/Tga/SimHash/Fingerprint.php

     * @var float
     */
-    protected $decimalValue;
+    protected $value;


Could you make this property private as well? I don't think it should be exposed anyway.

tgalopin · 2017-09-19T20:20:37Z

lib/Tga/SimHash/Tokenizer/String128Tokenizer.php

 */
 class String128Tokenizer implements TokenizerInterface
 {
+


Please remove this extra line.

tgalopin · 2017-09-19T20:20:52Z

lib/Tga/SimHash/Tokenizer/String256Tokenizer.php

+ */
+class String256Tokenizer implements TokenizerInterface
+{
+


Please remove this extra line.

tgalopin · 2017-09-19T20:21:04Z

lib/Tga/SimHash/Tokenizer/String512Tokenizer.php

+ */
+class String512Tokenizer implements TokenizerInterface
+{
+


Please remove this extra line.

nicolaichuk · 2017-09-28T21:21:54Z

@tgalopin
ping...

tgalopin

Travis is failing due to scalar type hints.

bbalet · 2018-08-20T09:41:34Z

lib/Tga/SimHash/Tokenizer/String128Tokenizer.php

-        return str_pad(base_convert(md5($element), 16, 2), 128, '0', STR_PAD_LEFT);
+        $hash = md5($element);
+        $hash = str_replace(self::$search, self::$replace, $hash);
+        $hash = str_pad($hash, 512, '0', STR_PAD_LEFT);


I think there is a typo here. Shouldn't you write:

$hash = str_pad($hash, 128, '0', STR_PAD_LEFT);

@bbalet, fixed.

bbalet · 2018-08-20T09:45:14Z

Hi guys,

I know what is to maintain an OSS project, but this PR looks good to me except a typo and the scalar type hint (except if you want to stop to maintain compatibility with older projects).

It is a good lib that helped me in the past, but now I am stuck with the error in 128 hashes.

How can I help?

tgalopin · 2018-08-20T10:04:20Z

Hello @bbalet !

The issue is that as @nicolaichuk didn't gave me access to code modification in this PR, I can't update his code, and using it without crediting him does not feel right in open-source :) . If you want to open a new PR based on his code, fixing the issues and adding a note in your commit message about @nicolaichuk, please don't hesitate to do so and I will review it happily :) !

nicolaichuk · 2018-08-21T12:38:07Z

@tgalopin

he issue is that as @nicolaichuk didn't gave me access to code modification in this PR

Option "Allow edits from maintainers" is selected in this PR.

@bbalet

by code review @bbalet tgalopin#5 (comment)

nicolaichuk added 2 commits September 14, 2017 21:45

fix 128 bit SimHash

34f1cfc

add 256 and 512 bit SimHash

small bug fix

3c70177

tgalopin requested changes Sep 14, 2017

View reviewed changes

* change comment

2938ad5

* add an ending line to this file

change protected to private

776d0d7

tgalopin reviewed Sep 19, 2017

View reviewed changes

fix by code review tgalopin#5

896912d

tgalopin requested changes Sep 29, 2017

View reviewed changes

bbalet reviewed Aug 20, 2018

View reviewed changes

nicolaichuk added 2 commits August 21, 2018 18:10

support php 5.3+

284477d

fix function call

b263fc7

by code review @bbalet tgalopin#5 (comment)

nicolaichuk requested a review from tgalopin November 17, 2021 16:54

Fix 128bit SimHash, and add 256bit and 512bit SimHash #5

Are you sure you want to change the base?

Fix 128bit SimHash, and add 256bit and 512bit SimHash #5

Uh oh!

Conversation

nicolaichuk commented Sep 14, 2017

Uh oh!

tgalopin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolaichuk commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgalopin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolaichuk commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgalopin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolaichuk Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbalet commented Aug 20, 2018

Uh oh!

nicolaichuk commented Sep 19, 2017 •

edited

Loading

nicolaichuk commented Sep 28, 2017 •

edited

Loading

nicolaichuk Aug 21, 2018 •

edited

Loading

nicolaichuk commented Aug 21, 2018 •

edited

Loading