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

WIP: upgrade to SymbolicUtils w/ fast terms #720

Merged
merged 27 commits into from
Jan 12, 2021
Merged

WIP: upgrade to SymbolicUtils w/ fast terms #720

merged 27 commits into from
Jan 12, 2021

Conversation

YingboMa
Copy link
Member

@YingboMa YingboMa commented Jan 7, 2021

This PR also deprecates .op and .args on Terms.

Most of the functionalities are upgraded except Hessian sparsity. @shashi is looking into that.

@ChrisRackauckas
Copy link
Member

Is this breaking? @YingboMa take a look at Pumas and make sure it stays fine.

@isaacsas
Copy link
Member

isaacsas commented Jan 7, 2021

We'll need some Catalyst updates too.

@YingboMa
Copy link
Member Author

YingboMa commented Jan 7, 2021

Yeah, it's a little breaking, unfortunately. I will test those when this PR and the SymbolicUtils PR are finalized.

@ChrisRackauckas
Copy link
Member

Yeah so this probably qualifies for a breaking release.

src/utils.jl Outdated Show resolved Hide resolved
@ChrisRackauckas
Copy link
Member

Yeah no worries. I am fine with having it a bit common in this stage in the repo, especially for a 100x performance gain. It just needs to be done right and take care of our downstream buddies

@YingboMa
Copy link
Member Author

YingboMa commented Jan 8, 2021

If we are going to do a breaking release anyway. Let's also sneak in #684 :P

@YingboMa
Copy link
Member Author

YingboMa commented Jan 9, 2021

All tests pass locally for me now.

@shashi
Copy link
Collaborator

shashi commented Jan 9, 2021

@isaacsas @ChrisRackauckas (ping anyone else using this package downstream)

This is a breaking change! Let me summarize what this means to ModelingToolkit users:

  • Term is not the only expression type now, there is also Add, Mul, and Pow.
  • If you were dispatching on Term, now use istree(x) to check if x is of any of these expression types. All of them support operation and arguments accessors, so:
  • x.op and x.args are deprecated in favor of operation(x) and arguments(x).
    Use these sed commands to make make this change across all files:
    • sed -i 's/\(\b[a-zA-Z0-9_]*\b\)\.args/arguments\(\1\)/g' **/*.jl
    • sed -i 's/\(\b[a-zA-Z0-9_]*\b\)\.op/operation\(\1\)/g' **/**/*.jl
      Note that expr.args where expr is an Expr will also be rewritten by this change, so be careful of what you commit (use git add -p to select the right changes, commit, and then run git reset --hard -- or however you usually do this.).

Here is what changed (and a migration guide) in SymbolicUtils.jl JuliaSymbolics/SymbolicUtils.jl#154 (comment) if you need to get into the finer details.

@ChrisRackauckas
Copy link
Member

Thanks, makes sense. Let's make sure own downstream libraries all get PRs too.

@YingboMa
Copy link
Member Author

YingboMa commented Jan 9, 2021 via email

@ChrisRackauckas
Copy link
Member

Sounds good.

@YingboMa
Copy link
Member Author

YingboMa commented Jan 11, 2021

It's the annoying string LaTeX test again... Can we just delete it to test for nonempty string? Or only looking for some patterns.

@ChrisRackauckas
Copy link
Member

what's going on there?

@YingboMa
Copy link
Member Author

Oh, this is actually due to the exp change. I will fix it.

@YingboMa
Copy link
Member Author

Fingers crossed. CI should pass in this commit.

@YingboMa YingboMa merged commit 135821e into master Jan 12, 2021
@YingboMa YingboMa deleted the s/addmulpow branch January 12, 2021 07:01
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