Adding optional argument to `create_child`
Includes the addition of a mutation_choice to create_child since 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 an llm_{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_choice but we might want to generalize this. I'm not sure what would be the correct API to expose for this?
Thanks! I will think more about the right API.
@codex review
💡 Codex Review
https://github.com/MilesCranmer/SymbolicRegression.jl/blob/30bd293045688eb07f4b269a3c01850656c20e68/src/SearchUtils.jl#L833-L834
Handle strip_metadata for custom population members
The new _parse_guesses_impl now constructs members of an arbitrary P<:AbstractPopMember, but the very next step still calls strip_metadata(member, ...). strip_metadata only has methods for PopMember, so if the user supplies a custom popmember_type this call will hit a MethodError before guesses can be processed (same issue will surface in other call sites that strip metadata). Consider adding an overload of strip_metadata for AbstractPopMember (using create_child or similar) so custom population members can survive the warm‑start path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
- @codex fix this CI failure
- @codex address that feedback
The new _parse_guesses_impl now constructs members of an arbitrary P<:AbstractPopMember, but the very next step still calls strip_metadata(member, ...). strip_metadata only has methods for PopMember, so if the user supplies a custom popmember_type this call will hit a MethodError before guesses can be processed (same issue will surface in other call sites that strip metadata). Consider adding an overload of strip_metadata for AbstractPopMember (using create_child or similar) so custom population members can survive the warm‑start path.
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?
ERROR: LoadError: failed process: Process(`/home/asehgal/.julia/juliaup/julia-1.11.7+0.x64.linux.gnu/bin/julia -C native -J/home/asehgal/.julia/juliaup/julia-1.11.7+0.x64.linux.gnu/lib/julia/sys.so --check-bounds=yes -g1 --startup-file=no --startup-file=no /home/asehgal/lasr-experiments/SymbolicRegression.jl/test/test_jet.jl`, ProcessExited(1)) [1]
Stacktrace:
[1] pipeline_error
@ ./process.jl:598 [inlined]
[2] run(::Cmd; wait::Bool)
@ Base ./process.jl:513
[3] run(::Cmd)
@ Base ./process.jl:510
[4] top-level scope
@ ~/lasr-experiments/SymbolicRegression.jl/test/runtests.jl:187
[5] eval
@ ./boot.jl:430 [inlined]
[6] include_string(mapexpr::typeof(identity), mod::Module, code::String, filename::String)
@ Base ./loading.jl:2734
[7] include_string(m::Module, txt::String, fname::String)
@ Base ./loading.jl:2744
[8] #invokelatest#2
@ ./essentials.jl:1055 [inlined]
[9] invokelatest
@ ./essentials.jl:1052 [inlined]
[10] (::TestItemServer.var"#9#13"{String, Nothing, Vector{TestItemServer.CoverageTools.FileCoverage}})()
@ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:352
[11] withpath(f::TestItemServer.var"#9#13"{String, Nothing, Vector{TestItemServer.CoverageTools.FileCoverage}}, path::String)
@ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:61
[12] run_testitem(endpoint::TestItemServer.JSONRPC.JSONRPCEndpoint{Base.PipeEndpoint, Base.PipeEndpoint}, params::TestItemServer.TestItemServerProtocol.RunTestItem, mode::String, coverage_root_uris::Nothing, state::TestItemServer.TestProcessState{var"#1#2"})
@ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:344
[13] runner_loop(state::TestItemServer.TestProcessState{var"#1#2"})
@ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:658
[14] (::TestItemServer.var"#33#35"{TestItemServer.TestProcessState{var"#1#2"}})()
@ TestItemServer ~/.vscode-server/extensions/julialang.language-julia-1.149.2/scripts/packages/TestItemControllers/testprocess/TestItemServer/src/TestItemServer.jl:697
in expression starting at /home/asehgal/lasr-experiments/SymbolicRegression.jl/test/runtests.jl:187
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.
ok done. passes the test cases as well!