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

Add support for simple deduplication of terms (for improved performance) when building functions #698

Closed
wants to merge 4 commits into from
Closed

Add support for simple deduplication of terms (for improved performance) when building functions #698

wants to merge 4 commits into from

Conversation

dpad
Copy link
Contributor

@dpad dpad commented Dec 24, 2020

This adds a deduplicate_terms argument to build_function() that allows the user to provide a list of Terms that they want to "deduplicate". Essentially, these Terms will be computed upfront before the system equations, and their values are substituted in to the system equations, thereby potentially saving a lot of extra processing. These are computed inside the iip/oop's let block, so the user has access to their variable names (e.g. they can use t).

A test is provided demonstrating the use and showing the speed up afforded.

NOTE: I've only added support for non-parallel, non-scalar output Julia-target functions. It will throw an error if a user tries to provide both a parallel/multithread and deduplicate_terms.

@ChrisRackauckas
Copy link
Member

@YingboMa what about using this for observed?

deduplicate is a bad name. intermediate_expressions or something of the sort could work. But this should usually be handled by the observed mechanism IMO unless someone is working at a lower level.

@@ -0,0 +1,46 @@
using Revise # DELETE ME
Copy link
Member

Choose a reason for hiding this comment

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

.

@dpad
Copy link
Contributor Author

dpad commented Dec 24, 2020

Thanks @ChrisRackauckas for your comment, I agree the name is bad. I am not sure what you are referring to about observed but I'll leave this PR open for you to use/modify as you wish (I am just using this + #681 for some work I'm doing at the moment).

@ChrisRackauckas
Copy link
Member

Yes no worries. I'm going to ask Chris Foster for help finishing the other PR once the holidays are completed, and for here I think it's up to @YingboMa and @shashi as they are digging through build_function and intermediate calculations. I'll follow this but they are all in the weeds so I'll defer to their opinions.

ode_function_deduplicated = ODEFunction(ode_system; jac=false, tgrad=false, eval_expression=false, deduplicate_terms=dupe_terms)

# Run both functions and compare...
u0 = rand(Float64, length(x))
Copy link
Member

Choose a reason for hiding this comment

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

x is not defined anywhere. I suggest you to use @safetestset to run the tests locally, so that you won't pollute your test environment.

@YingboMa
Copy link
Member

I don't think pushing dup terms into a let block is extendable. The expression could be

x = expensivefun1()
y = x + expensivefun2()

and a let block cannot handle that. In general, some kind of dependency analysis is also needed here. If you don't need this feature urgently, I'd suggest to wait for a more general handling of observed.

@dpad
Copy link
Contributor Author

dpad commented Feb 2, 2021

@YingboMa @ChrisRackauckas Any news on the idea of observed? Anywhere I can look to track or help out on it? This specific fix was pretty critical for performance in one of my models (up to ~20x faster).

@ChrisRackauckas
Copy link
Member

@YingboMa and @shashi are finishing it up this week, and #764 is a major change here.

@ChrisRackauckas
Copy link
Member

We discussed all of this and for performance, it's best to do this automatically. What you're calling "de-duplicate" is common subexpression elimination (CSE) which the next generation of build_function has planned to have a switch for to do in full. See JuliaSymbolics/SymbolicUtils.jl#121 and JuliaSymbolics/SymbolicUtils.jl#200 . Given those pieces, I'll be closing this but let me know if you have a use case where directly doing CSE by hand for only a few chosen terms is necessary, as the compiler style will automatically apply it to all cases.

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.

3 participants