-
Notifications
You must be signed in to change notification settings - Fork 154
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.
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
A fix for occursin
#1139
A fix for occursin
#1139
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
Now replaces the |
Rebase. |
|
||
function Base.replace(expr::Symbolic, rules...) | ||
_replace(unwrap(expr), rules...) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought replace was fine though right? The occursin was the one which we couldn't define the right methods for? Either way, I'm fine with this change, but it would be good to add a deprection method for replace, I believe David Sanders uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, my recollection from our discussion was that the function we are writing here is different from the normal replace
and we should rename it to replacenode
(it was a while ago, so do not remember exactly). Personally, I have no strong opinion.
In an unrelated issue (and more serious), the current replace
is also broken (I think after the latest term interface update): #1153
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that applies to occursin, not so much to replace.
Look for the next release that just got merged for the term interface fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I update this PR to use replace
instead of replacenode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixpoint
keyword arg is not in these top level methods, it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update that as well?
Update:
|
When I tried to check if old |
@shashi There is a near instant error, but that seems to be due to a package incompatibility error:
This PR does not changes any project files, so should be related to master Symbolics? |
Rebase |
Ok, this passes now, exept for:
Small note, currently the 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. hasnode(r::Union{Function, Num, Symbolic}, y::Number) = false |
Looks like this PR broke it. |
MTK downstream still fail due to some incompatibility, but otherwise all good. |
The current implementation causes occasional ambiguity errors. This fixes that.