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

goimagehash: Implement PerceptionHashExtend #18

Merged
merged 6 commits into from
Feb 8, 2019

Conversation

TokyoWolFrog
Copy link
Contributor

add a function that returns a hash computation of phash which the size can be set larger than uint64

add a function that returns a hash computation of phash which the size can be set larger than uint64
@coveralls
Copy link

coveralls commented Feb 4, 2019

Pull Request Test Coverage Report for Build 84

  • 76 of 98 (77.55%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-5.08%) to 87.919%

Changes Missing Coverage Covered Lines Changed/Added Lines %
hashcompute.go 21 23 91.3%
imagehash.go 55 75 73.33%
Totals Coverage Status
Change from base Build 60: -5.08%
Covered Lines: 262
Relevant Lines: 298

💛 - Coveralls

@corona10
Copy link
Owner

corona10 commented Feb 4, 2019

@TokyoWolFrog

Hi, Thank you for the contribution, this PR will be a lot of improvement for goimagehash.

There are some things that I want to achieve with extension hash.

  • We need a way to calculate a distance of two hashes which are calculated by PerceptionHashExtend.
  • We need a unit test

So my suggestions are these.

  • Implement BigImageHash/ExImageHash/(:name what you want) which can compare with other bighash.
// BigImageHash is a struct of hash computation.
type BigImageHash struct {
	hash string
	kind Kind
}
  • Add unit tests for PerceptionHashExtend. You can use below codes to check whether bighash left the same result by using the same file on 64bits size case.

func TestHashCompute(t *testing.T) {

Can you please reflect my comments?
Again thank you!

@TokyoWolFrog
Copy link
Contributor Author

@corona10
Thanks for reply! It will take some time, but I would like to reflect your comments.

@corona10
Copy link
Owner

corona10 commented Feb 6, 2019

@TokyoWolFrog

Thanks alot!

Copy link
Owner

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Oh and the furthermore
Please add your name on https://github.com/corona10/goimagehash/blob/master/AUTHORS.md
and also add on here https://github.com/corona10/goimagehash/blob/master/CODEOWNERS when you implement extension hash.
Thank you!

h2 := &ExtImageHash{hash: []uint64{0x678be53815e510f7}} // 8 bits flipped

for i := 0; i < b.N; i++ {
h1.Distance(h2)

Choose a reason for hiding this comment

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

Error return value of h1.Distance is not checked (from errcheck)

@TokyoWolFrog
Copy link
Contributor Author

@corona10
Hi,

  • add distance calculation of two hashes calculated by PerceptionHashExtend.
  • add some unit test
    To make the new function more consistent with the original code style, I made some changes of the implement.

Copy link
Owner

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@TokyoWolFrog
I left some comments! Most of the things look great!
Most important thing is that we should align the API signature of the hash structure.

imagehash.go Outdated Show resolved Hide resolved
imagehash.go Outdated Show resolved Hide resolved
imagehash.go Outdated Show resolved Hide resolved
@corona10
Copy link
Owner

corona10 commented Feb 7, 2019

Also we need to add func (h *ImageHash) GetHash() []uint64

func (h *ImageHash) GetHash() uint64 {
to get Hash value([]uint64)

imagehash.go Outdated Show resolved Hide resolved
@TokyoWolFrog
Copy link
Contributor Author

@corona10
Thanks for the review!
I modified the code to align the API signature with #4

Copy link
Owner

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@TokyoWolFrog

LGTM to me!!

Just fix the issue which Golang CI mentioned.
#18 (comment)

Everything looks great! Thanks alot

@corona10 corona10 changed the title add PerceptionHashExtend function goimagehash: Implement PerceptionHashExtend Feb 8, 2019
@corona10 corona10 merged commit c41a59a into corona10:master Feb 8, 2019
@corona10 corona10 mentioned this pull request Feb 11, 2019
2 tasks
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.

4 participants