Skip to content

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

Open
MilesCranmerBot wants to merge 6 commits intoSymbolicML:release-v1from
MilesCranmerBot:bot/backport-fix-im-string-parse-release-v1
Open

fix: backport im string parsing fix to release-v1#169
MilesCranmerBot wants to merge 6 commits intoSymbolicML:release-v1from
MilesCranmerBot:bot/backport-fix-im-string-parse-release-v1

Conversation

@MilesCranmerBot
Copy link
Contributor

@MilesCranmerBot MilesCranmerBot commented Mar 8, 2026

Backport of #168 to release-v1.

Includes the im parsing fix and a small release-v1 compatibility adjustment for the new parse test.

Validation run locally:

  • targeted check for parse_expression("0.1im + x") and parse_expression("im + x2") with assertions on type/evaluation (ok)

I did not wait for another full Pkg.test() pass after the final compatibility patch because it is relatively heavy in this environment.

@MilesCranmer
Copy link
Member

@MilesCranmerBot CI is failing

Co-authored-by: Miles Cranmer <miles.cranmer@gmail.com>
Comment on lines +261 to +262
@unstable parse_expression(ex::String; kws...) = parse_expression(Meta.parse(ex); kws...)

Copy link
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

Copy link
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.

Copy link
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.

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.

2 participants