Skip to content

fix: backport im string parsing fix to release-v1 #169

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.

Sign up for GitHub

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

Open
MilesCranmerBot wants to merge 6 commits into
base: release-v1
Choose a base branch
from
Open

fix: backport im string parsing fix to release-v1 #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
741031c
fix: treat im as imaginary unit in string parsing
MilesCranmerBot Mar 4, 2026
c5f6bba
fix: address review suggestions for im parse
MilesCranmerBot Mar 4, 2026
63d7900
style: run JuliaFormatter
MilesCranmerBot Mar 4, 2026
e835279
style: remove unnecessary inline parse helpers
MilesCranmer Mar 8, 2026
59815bb
fix: adapt im parsing backport for release-v1
MilesCranmerBot Mar 8, 2026
be27550
fix: repair parse test traversal for Node on release-v1
MilesCranmerBot Mar 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/Parse.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,21 @@ end
)
end

_replace_imaginary_unit_symbol(ex) = ex
@unstable _replace_imaginary_unit_symbol(ex::Symbol) = ex === :im ? im : ex
function _replace_imaginary_unit_symbol(ex::Expr)
return Expr(ex.head, map(_replace_imaginary_unit_symbol, ex.args)...)
end

@unstable function _normalize_expression_for_parse(
ex, variable_names::Union{AbstractVector{<:AbstractString},Nothing}
)
if variable_names !== nothing && ("im" in variable_names)
return ex
end
return _replace_imaginary_unit_symbol(ex)
end

"""Parse an expression Julia `Expr` object."""
@unstable function parse_expression(
ex;
Expand Down Expand Up @@ -237,11 +252,14 @@ end
operators
end

ex = _normalize_expression_for_parse(ex, variable_names)
tree = _parse_expression(ex, operators, variable_names, N, E, evaluate_on; kws...)
return constructorof(E)(tree; operators, variable_names, kws...)
end
end

@unstable parse_expression(ex::String; kws...) = parse_expression(Meta.parse(ex); kws...)

Comment on lines +261 to +262

@MilesCranmer MilesCranmer Mar 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a new feature rather than a backport? Can you clarify @MilesCranmerBot

@MilesCranmerBot MilesCranmerBot Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep — it looks like a feature addition in release-v1, but it’s actually a straight backport of what already landed on master.

On master the fix is introduced in commit 741031c4 (“fix: treat im as imaginary unit in string parsing”) and includes:

  • normalizing :im → the Julia constant im when parsing (unless "im" is explicitly listed in variable_names, in which case it stays a variable), and
  • the additive convenience overload parse_expression(::String) (also present on master).

I included the ::String overload here only to keep release-v1 behavior/API aligned with master for this parsing path; if you’d prefer the backport to be strictly minimal, I’m happy to drop that overload and have the test call Meta.parse(...) explicitly instead.

@MilesCranmerBot MilesCranmerBot Mar 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep — this PR is meant to be a straight backport of #168 (merged to master).

The only extras here are release-v1-specific adaptation:

  1. release-v1 didn’t have the convenience method parse_expression(::String) that master already has. I added it as a tiny wrapper around Meta.parse so the same regression test can run. It doesn’t enable anything you couldn’t already do via parse_expression(Meta.parse(str); ...).
  2. The new test’s tree-walk helper is written against the release-v1 node layout (v1 uses l/r instead of the children representation on master).

The actual behavior change being backported is just normalizing the :im symbol to Julia’s imaginary unit constant unless "im" is explicitly listed in variable_names (in which case it remains a variable).

If you’d prefer to keep release-v1’s public API surface strictly frozen, I can drop the parse_expression(::String) method and update the test to call Meta.parse explicitly.

"""An empty module for evaluation without collisions."""
module EmptyModule end

Expand Down
45 changes: 45 additions & 0 deletions test/test_parse.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,51 @@
end
end

@testitem "String parse treats Julia imaginary unit `im` as a constant" begin
using DynamicExpressions
using Test

operators = OperatorEnum(;
binary_operators=[+, -, *, /], unary_operators=[], define_helper_functions=false
)

ex = parse_expression("0.1im + x"; operators, variable_names=["x"])
@test typeof(ex) <: Expression{ComplexF64}

function count_vars(n)
if n.degree == 0
return n.constant ? 0 : 1
elseif n.degree == 1
return count_vars(n.l)
else
return count_vars(n.l) + count_vars(n.r)
end
end

@test count_vars(ex.tree) == 1

# Check evaluation + eltype
Xr = reshape([1.0], 1, :)
yr = ex(Xr)
@test eltype(yr) == ComplexF64
@test yr[1] ≈ 1.0 + 0.1im

Xc = reshape([1.0 + 2.0im], 1, :)
yc = ex(Xc)
@test eltype(yc) == ComplexF64
@test yc[1] ≈ 1.0 + 2.1im

# If `"im"` is in `variable_names`, it should be treated as a variable
ex2 = parse_expression("im + x2"; operators, variable_names=["im", "x2"])
@test typeof(ex2) <: Expression
@test count_vars(ex2.tree) == 2

X2 = reshape(Float32[2, 3], 2, :)
y2 = ex2(X2)
@test eltype(y2) == Float32
@test y2[1] == 5.0f0
end

@testitem "Can also parse just a float" begin
using DynamicExpressions
operators = OperatorEnum() # Tests empty operators
Expand Down
Loading