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

Update formative_memories.py #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jshepp27
Copy link

Going to win awards for the smallest contribution on the planet!!! - I will be back with a bigger contribution soon though ;)!

It seems splitting lines over "\n\n\n" isn't robust enough to ensure the aggregated formative episodes split into individual episodes, to then be zipped into memories.

When I run it with a "\n\n" separator, my experiments including the example "three_key_questions" run without the warning from episodes == DEFAULT_FORMATIVE_AGES.

Of course, given how sensitive this is, it may make sense to implement a more robust fix. This works repeatedly for me.

Going to win awards for the smallest contribution on the planet!!! - I will be back with a bigger contribution soon though ;)! 

It seems splitting lines over "\n\n\n" isn't robust enough to ensure the aggregated formative episodes split into individual episodes, to then be zipped into memories. 

When I run it with a "\n\n" separator, my experiments including the example "three_key_questions" run without the warning from episodes == DEFAULT_FORMATIVE_AGES.

Of course, given how sensitive this is, it may make sense to implement a more robust fix. This works repeatedly for me.
Copy link

google-cla bot commented Apr 10, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jshepp27 jshepp27 closed this Apr 10, 2024
@jshepp27 jshepp27 reopened this Apr 10, 2024
@jshepp27
Copy link
Author

@jzleibo - looks like it's blocked. Open suggestion for you! The example three_key_questions.ipynb is incorrectly loading formative memories otherwise.

Great piece of work by the way! The python is beautiful.

@jzleibo
Copy link
Collaborator

jzleibo commented Apr 11, 2024

@jzleibo - looks like it's blocked. Open suggestion for you! The example three_key_questions.ipynb is incorrectly loading formative memories otherwise.

Great piece of work by the way! The python is beautiful.

I think it was only blocked because you have not accepted the Google Contributor License agreement. I believe it should have been sent to you by email when you submitted this pull request on github.

As for the change itself, it looks fine to me. @jagapiou , @vezhnick , what do you think?

Which LLM did you use that had issues consistently outputting '\n\n\n'?

@jshepp27
Copy link
Author

Ah, noted. I'll try accepting the CLA again for future contributions.

That's a very good point - I didn't think about variation over LLMs. Nonetheless, GPT-4 via the API not azure.

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