Move HNSW parameters to the HnswGraphBuilder class

[dantuzi]

Description

The Hnsw parameters like DEFAULT_MAX_CONN, and DEFAULT_BEAM_WIDTH should not be maintained in the codec-related class HnswVectorFormat but it's more reasonable to keep them in the HnswGraphBuilder class.

[alessandrobenedetti]

It seems reasonable to me, the max_conn and beam_width are used by (and passed as parameters) the graph builder.
It's ok to me to move the Defaults in there

[msokolov]

Sorry, I don't see this being any better than the current situation; aside from tests, the parameters are only used in HnswVectorsFormat where they are currently defined, so I think we should leave them there.

[dantuzi]

@msokolov I'm finalizing another PR that works with HnswGraph and, to create the graph, I use the constants DEFAULT_MAX_CONN and DEFAULT_BEAM_WIDTH already defined. So, in the future, these constants will also be used outside the class HnswVectorsFormat.

[alessandrobenedetti]

@msokolov do you see any reason why we shouldn't do it? because I reviewed the pull request Daniele is going to publish (Word2Vec for synonyms generation: https://www.youtube.com/watch?v=rKYJQhZxQFQ&t=469s) and having those constants in the HNSW Graph builder facilitates the things.
Furthermore, those constants in terms of responsibility affect how the graph is built(more than the codec), so in terms of cohesion of the class, it seems reasonable to me for them to be in the HNSW Graph builder.

[msokolov]

I would say let's make the changes when we can see the purpose of doing it? So, if Daniele is planning to make use of this in an upcoming PR, let's just incorporate the change into that?

…

On Wed, Jan 25, 2023 at 10:22 AM Alessandro Benedetti < ***@***.***> wrote: @msokolov <https://github.com/msokolov> do you see any reason why we shouldn't do it? because I reviewed the pull request Daniele is going to publish (Word2Vec for synonyms generation: https://www.youtube.com/watch?v=rKYJQhZxQFQ&amp;t=469s) and having those constants in the HNSW Graph builder facilitates the things. Furthermore, those constants in terms of responsibility affect how the graph is built(more than the codec), so in terms of cohesion of the class, it seems reasonable to me for them to be in the HNSW Graph builder. — Reply to this email directly, view it on GitHub <#12048 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHHUQLHCOE5VCFT7IE7JUDWUD5EPANCNFSM6AAAAAATM745TY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[alessandrobenedetti]

Ok, that's fine! @dantuzi you can cancel this PR and just include the change in the final PR.
Thanks @msokolov for the suggestion!