-
Notifications
You must be signed in to change notification settings - Fork 116
Adding optional argument to create_child
#515
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
base: abstract-popmember
Are you sure you want to change the base?
Adding optional argument to create_child
#515
Conversation
…l into abstract-popmember
…hild and incorporating it in Mutate.jl
create_child
|
Thanks! I will think more about the right API. @codex review |
This comment was marked as resolved.
This comment was marked as resolved.
looking into this... Seems like I'll need to make a new constructor for subtypes of AbstractPopMember |
|
Ok. Running all test cases locally right now but this passes the test_abstract_popmember.jl test suite. |
|
Ok. Test cases pass locally. I can't get the JET tests to work properly. Any ideas why this happens? |
|
Sorry I tried to mark that codex review as "resolved" because I disagreed with it, my bad. But we actually want to keep strip_metadata specific to PopMember as a way to force downstream types to reimplement it. It's a bit safer than trying to come up with some automatic generic approach |
|
Ah gotcha. Let me revert the changes and add it to the docs. |
73762fb to
230279d
Compare
|
ok done. passes the test cases as well! |
Includes the addition of a
mutation_choicetocreate_childsince in many cases the child creation is dependent on the type of mutation (e.g. if we are tracking the number of llm calls, we need to see if mutation_choice was anllm_{choice})One point that needs further discussion: Do we want to generalize this pattern? For my use case in LaSR all I need is the
mutation_choicebut we might want to generalize this. I'm not sure what would be the correct API to expose for this?