Skip to content

Conversation

svilupp
Copy link
Collaborator

@svilupp svilupp commented Aug 18, 2024

I've made a few small tweaks.

@splendidbug can you check if the README makes sense here?

In addition, why do you show "embedding_dimension" 1024? What is the default setting you use? Because it seems that it's 1024+Bool, which is okay but it would at least deserve 1-2 sentences to explain the default or the choices users have?

WDYT?

@svilupp svilupp requested a review from splendidbug August 18, 2024 14:17
@splendidbug
Copy link
Collaborator

I've made a few small tweaks.

@splendidbug can you check if the README makes sense here?

In addition, why do you show "embedding_dimension" 1024? What is the default setting you use? Because it seems that it's 1024+Bool, which is okay but it would at least deserve 1-2 sentences to explain the default or the choices users have?

WDYT?

The defaults are 3072 and Bool. I'll change the default to 3072 and Float32. I'll also add the default values in README. Thanks!

## Building the Index
```julia
index = make_knowledge_packs(; single_urls=["https://docs.sciml.ai/Overview/stable/"], index_name="sciml", embedding_size=1024)
index = make_knowledge_packs(["https://docs.sciml.ai/Overview/stable/"]; index_name="sciml", embedding_size=1024, bool_embeddings=true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks good! I would suggest making them consistent, eg, embedding_size, embedding_bool` (it's easier for users to remember)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how hard it is to change it in the user-facing functions, but the kwarg name in AIHelpMe is embedding_dimension (not size)

Copy link

codecov bot commented Aug 20, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@splendidbug
Copy link
Collaborator

I'm merging the PR. I'll update kwarg names and the same will be reflected in readme in the next PR

@splendidbug splendidbug merged commit 04977ec into main Aug 20, 2024
2 checks passed
@splendidbug splendidbug deleted the svilupp-readme branch August 20, 2024 08:50
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.

2 participants