Skip to content
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

Merged
merged 7 commits into from
Jun 4, 2024
Merged

A fix for occursin #1139

merged 7 commits into from
Jun 4, 2024

Conversation

TorkelE
Copy link
Contributor

@TorkelE TorkelE commented May 14, 2024

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

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2024

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 ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

@TorkelE
Copy link
Contributor Author

TorkelE commented May 14, 2024

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

Rebase.


function Base.replace(expr::Symbolic, rules...)
_replace(unwrap(expr), rules...)
end
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

@TorkelE
Copy link
Contributor Author

TorkelE commented Jun 3, 2024

Update:

  • rebased.
  • change repalcenode to replace.
  • make fixpoint = false an optional argument to the replace function (rather than being in the internal _replace).

@TorkelE
Copy link
Contributor Author

TorkelE commented Jun 3, 2024

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

TorkelE commented Jun 3, 2024

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

Rebase

@TorkelE
Copy link
Contributor Author

TorkelE commented Jun 3, 2024

Ok, this passes now, exept for:

  • MTK downstream, but again that is an incompatibility of packages in MTK.
  • This test: Build Targets Test | 2 1 3 2.2s

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

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

Looks like this PR broke it.

@TorkelE
Copy link
Contributor Author

TorkelE commented Jun 4, 2024

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

@shashi shashi merged commit 8498d6b into JuliaSymbolics:master Jun 4, 2024
10 of 11 checks passed
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.

4 participants