You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/12/30 16:28:23 UTC
[GitHub] [lucene] dantuzi opened a new pull request, #12048: Move HNSW parameters to the HnswGraphBuilder class
dantuzi opened a new pull request, #12048:
URL: https://github.com/apache/lucene/pull/12048
### Description
The Hnsw parameters like MAXIMUM_MAX_CONN, DEFAULT_MAX_CONN, MAXIMUM_BEAM_WIDTH, 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on pull request #12048: Move HNSW parameters to the HnswGraphBuilder class
Posted by GitBox <gi...@apache.org>.
msokolov commented on PR #12048:
URL: https://github.com/apache/lucene/pull/12048#issuecomment-1368477965
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on pull request #12048: Move HNSW parameters to the HnswGraphBuilder class
Posted by "alessandrobenedetti (via GitHub)" <gi...@apache.org>.
alessandrobenedetti commented on PR #12048:
URL: https://github.com/apache/lucene/pull/12048#issuecomment-1403387891
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dantuzi commented on pull request #12048: Move HNSW parameters to the HnswGraphBuilder class
Posted by GitBox <gi...@apache.org>.
dantuzi commented on PR #12048:
URL: https://github.com/apache/lucene/pull/12048#issuecomment-1380490660
@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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on pull request #12048: Move HNSW parameters to the HnswGraphBuilder class
Posted by "alessandrobenedetti (via GitHub)" <gi...@apache.org>.
alessandrobenedetti commented on PR #12048:
URL: https://github.com/apache/lucene/pull/12048#issuecomment-1403736061
Ok, that's fine! @dantuzi you can cancel this PR and just include the change in the final PR.
Thanks @msokolov for the suggestion!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dantuzi closed pull request #12048: Move HNSW parameters to the HnswGraphBuilder class
Posted by "dantuzi (via GitHub)" <gi...@apache.org>.
dantuzi closed pull request #12048: Move HNSW parameters to the HnswGraphBuilder class
URL: https://github.com/apache/lucene/pull/12048
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on pull request #12048: Move HNSW parameters to the HnswGraphBuilder class
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #12048:
URL: https://github.com/apache/lucene/pull/12048#issuecomment-1403475767
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&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
> <https://github.com/apache/lucene/pull/12048#issuecomment-1403387891>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAHHUQLHCOE5VCFT7IE7JUDWUD5EPANCNFSM6AAAAAATM745TY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org