fix: allow tracing custom operators in SymbolicUtils conversion#171
Draft
MilesCranmerBot wants to merge 3 commits intoSymbolicML:masterfrom
Draft
fix: allow tracing custom operators in SymbolicUtils conversion#171MilesCranmerBot wants to merge 3 commits intoSymbolicML:masterfrom
MilesCranmerBot wants to merge 3 commits intoSymbolicML:masterfrom
Conversation
When index_functions=false, try tracing non-whitelisted operators by calling them on symbolic children; only error on MethodError.\n\nAlso update SymbolicUtils tests to cover both traced custom operators and indexed round-tripping paths.\n\nCo-authored-by: Miles Cranmer <[email protected]>
Contributor
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #171 +/- ##
==========================================
+ Coverage 59.43% 59.46% +0.03%
==========================================
Files 30 30
Lines 2682 2689 +7
==========================================
+ Hits 1594 1599 +5
- Misses 1088 1090 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
@MilesCranmerBot needs fixes for formatting and for code coverage. i.e., make the diff coverage 100%. |
Comment on lines
+116
to
+123
| traced = try | ||
| op(sym_children...) | ||
| catch e | ||
| if e isa MethodError | ||
| throw(error("Unsupported operation $(op) in SymbolicUtils conversion")) | ||
| end | ||
| rethrow() | ||
| end |
Member
There was a problem hiding this comment.
This might be type unstable? i.e., might need to do
local traced
try
traced = op...
catch e
...
endCan you check if this is type unstable? Please show the inferred type in each instance (REPL output with Cthulhu.jl)
- add regression coverage for unsupported custom-op paths\n- ensure SymbolicUtils compat avoids Julia 1.12 precompile breakage\n\nCo-authored-by: Miles Cranmer <[email protected]>
Contributor
Author
|
Implemented ✅
Pushed to this PR branch in commit ae6c2a6. I also ran |
Member
|
@MilesCranmerBot there's still a downgrade compat failure despite assertion otherwise, please fix |
Co-authored-by: Miles Cranmer <[email protected]>
Contributor
Author
|
Fixed — the downgrade failure was coming from I replaced that line with a plain indexed inbounds loop:
This avoids the problematic Commit: 9558210a4e5f6ac896084ed0432ead5e11289a49 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #170
Summary
index_functions=falseby calling the operator on symbolic childrenindex_functions=truebehavior unchanged (operator indexing for round-trip preservation)MethodError, still raise a clear unsupported-operation errorTests
index_functions=falseindex_functions=truePkg.test()), which passed