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

Fix some typos/language in regex and sequence #257

Merged
merged 3 commits into from
Sep 2, 2023

Conversation

jrysana
Copy link
Contributor

@jrysana jrysana commented Aug 22, 2023

Please correct me if I'm wrong about any of the below! But I noticed a few more things that seem mismatched in comments.

  • Fixed a typo in regex/create_proposal implying that this method is only for integer generation, whereas it seems clear that this method is used for all schemas.
  • Fixed some typos where argument names were mismatched or nonexistent in the actual function code.
  • Fixed some typos for overall language (e.g. "used to compute" instead of "used to computes") and to clarify potentially confusing comments.
  • Fixed a broken link in README.md


Parameters
---------
model
The model to use to computes the next-token logits.
The model to use to compute the next-token logits.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a nitpick, but I think these comments for model args might be more welcoming to some readers with less context, without losing clarity, if we said "the language model" / "the text transformer model" or something, instead of just "the model" - being that "model" is a very vague word and requires previous context to understand. For example, just in this file alone, there is a connotation of "model" as a text transformer model and "model" as a Pydantic data validation model.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jrysana, that sounds like a good idea to me 🙂 would you want to make those changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, pushed @zaqqwerty

Copy link
Contributor

@zaqqwerty zaqqwerty left a comment

Choose a reason for hiding this comment

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

Hi @jrysana, thank you for the PR! We appreciate your thorough reading of the code. LGTM, lmk if you'd first like to update based on your suggestion before we merge.


Parameters
---------
model
The model to use to computes the next-token logits.
The model to use to compute the next-token logits.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jrysana, that sounds like a good idea to me 🙂 would you want to make those changes?

@zaqqwerty zaqqwerty merged commit 8b324ea into outlines-dev:main Sep 2, 2023
4 checks passed
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.

None yet

2 participants