JuliaSymbolics / Symbolics.jl

Symbolic programming for the next generation of numerical software
https://symbolics.juliasymbolics.org/stable/
Other
1.32k stars 146 forks source link

A fix for `occursin` #1139

Closed TorkelE closed 3 weeks ago

TorkelE commented 1 month ago

The current implementation causes occasional ambiguity errors. This fixes that.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 10.17%. Comparing base (79c4e92) to head (a8e987f). Report is 209 commits behind head on master.

Files Patch % Lines
src/rewrite-helpers.jl 0.00% 15 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1139 +/- ## =========================================== - Coverage 77.07% 10.17% -66.90% =========================================== Files 32 36 +4 Lines 3533 3616 +83 =========================================== - Hits 2723 368 -2355 - Misses 810 3248 +2438 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TorkelE commented 1 month ago

Now replaces the replace and occursin Symbolics functions with the replacenode and hasnode functions (within the context where these were used). This prevents some clashes with occursin defined for symbolic in SymboliUtils.

ChrisRackauckas commented 4 weeks ago

Rebase.

TorkelE commented 4 weeks ago

Update:

TorkelE commented 4 weeks ago

When I tried to check if old replace worked again, it didn't. However, that was still on SymbolicsUtils 1.6, as v2.0 is not compatible with the latest Symbolics.

TorkelE commented 4 weeks ago

@shashi There is a near instant error, but that seems to be due to a package incompatibility error:

ERROR: LoadError: Unsatisfiable requirements detected for package SymbolicUtils [d1185830]:
 SymbolicUtils [d1185830] log:
 ├─possible versions are: 0.1.0-2.0.2 or uninstalled
 ├─restricted to versions 2.0.2-2 by Symbolics [0c5d862f], leaving only versions: 2.0.2
 │ └─Symbolics [0c5d862f] log:
 │   ├─possible versions are: 5.29.0 or uninstalled
 │   └─Symbolics [0c5d862f] is fixed to version 5.29.0
 └─restricted by compatibility requirements with TermInterface [8ea1fca8] to versions: [0.1.0-0.14.0, 1.0.0-1.5.1] or uninstalled — no versions left
   └─TermInterface [8ea1fca8] log:
     ├─possible versions are: 0.1.0-1.0.0 or uninstalled
     └─restricted to versions 1 by Symbolics [0c5d862f], leaving only versions: 1.0.0
       └─Symbolics [0c5d862f] log: see above

This PR does not changes any project files, so should be related to master Symbolics?

ChrisRackauckas commented 4 weeks ago

Rebase

TorkelE commented 3 weeks ago

Ok, this passes now, exept for:

Small note, currently the hasnode and replace functions only work on symbolic expressions:

using Symbolics
@variables x y z
expr = x^2 + y
hasnode(x^2, expr) # true
Symbolics.replace(expr, x^2 => z) # y + z

however, if the expression is just a number, these fails:

expr = 10
hasnode(x^2, expr) # ERROR: MethodError: no method matching hasnode(::Num, ::Int64)
Symbolics.replace(expr, x^2 => z) # ERROR: MethodError: no method matching replace(::Int64, ::Pair{Num, Num})

if you were to apply these to e.g. lhs and rhs of an equation, you might run into a problem for e.f. 0 ~ x + y. Since replace is not a general function, maybe it should not be changed, but would it make sense to add a dispatch for hasnode:

hasnode(r::Union{Function, Num, Symbolic}, y::Number) = false
ChrisRackauckas commented 3 weeks ago

This test: Build Targets Test | 2 1 3 2.2s

Looks like this PR broke it.

TorkelE commented 3 weeks ago

MTK downstream still fail due to some incompatibility, but otherwise all good.