-
Notifications
You must be signed in to change notification settings - Fork 34
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
Oracle Language Model #316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part this looks good and pretty straightforward, just a couple of suggested changes/questions.
- It may be beneficial to add a method to update the target text, so we don't have to reinitialize the LM for every task. Probably not a huge deal since we aren't actually loading a model
- There should be some protections around the valid value range for target_bump (non-negative, some sort of max?).
- Do we need to normalize the distribution in predict() before returning?
- For the other LMs, backspace is not included in the distribution. The predict() method returns a probability of 0, and then it is set externally based on the parameters file. Can you run a test and check the session.json to make sure the oracle backspace isn't being overwritten by the static backspace_prob?
@dcgaines, thanks for the feedback! Good thinking on the target_bump range checks. I added those, as well as some setter validation to ensure that the task_text does not get a None value. I also added some unit tests to demonstrate how to update the target text. The normalization happens in the The other LM that returns a probability for backspace is the UniformLanguageModel. The CopyPhraseWrapper will only replace the value for the backspace if the LM did not return one or if the configured value for the min backspace probability is larger than what was returned by the LM. Currently that min value is set to 0.0. I checked the session.json from my Copy Phrase session using the OracleLanguageModel and confirmed that the value provided by the LM was not overridden. |
Thanks for those changes and checks. Another thing I just thought of is that the model should have some handling for if target_text == evidence. Theoretically, this shouldn't happen, since the phrase will be done, but it might be good to handle just in case, and it might be as simple as returning None from the next_target() function, so that nothing gets bumped and there is a uniform prob. |
Oh yes, good catch. I'll add some handling for that. |
…eds the task_text length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Overview
Added a new language model that has knowledge of the target copy phrase and boosts the probability of the next target.
Ticket
https://www.pivotaltracker.com/story/show/187083845
Contributions
Test