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.

Sign up for GitHub

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

Jump to bottom

Only permit scalars and weighted sums for assign #2562

Merged
merged 6 commits into from
Nov 18, 2022
Merged

Only permit scalars and weighted sums for assign #2562

merged 6 commits into from
Nov 18, 2022

Conversation

connorjward
Copy link
Contributor

@connorjward connorjward commented Sep 22, 2022 •
edited
Loading

Associated issue: #2556

We also need to merge changes in pyadjoint: dolfin-adjoint/pyadjoint#93.

wence-
wence- reviewed Sep 22, 2022
View reviewed changes
firedrake/assign.py Outdated Show resolved Hide resolved
wence-
wence- reviewed Sep 22, 2022
View reviewed changes
firedrake/assign.py Outdated Show resolved Hide resolved
wence-
wence- reviewed Sep 22, 2022
View reviewed changes
firedrake/assign.py Outdated Show resolved Hide resolved
wence-
wence- reviewed Sep 22, 2022
View reviewed changes
firedrake/assign.py Outdated Show resolved Hide resolved
@connorjward
Copy link
Contributor Author

A quick experiment shows that this can speed things up quite a bit. I've been running the DG advection demo on my branch vs master. Below are the flamegraphs I get on my laptop (with a hot cache). The first shows current master and the bottom one is my code. I found the code took ~25s on master and mine took ~20s. You can see that the assign bar is much smaller in the bottom one.

old1
new1

I have a few comments:

  • We still need to apply a tree visitor to the expression. Caching on a persistent expression would let us avoid this.
  • Alternatively, using a persistent Assigner object makes pretty much all overhead go away. The flamegraph below, where I tried this, assign takes 0.2s out of a total 18s.

persistent_assigner

@connorjward connorjward force-pushed the connorjward/assign-weighted-sums-only branch from 8f4dfab to 9c80185 Compare October 13, 2022 11:49
@connorjward connorjward changed the title WIP Only permit scalars and weighted sums for assign Only permit scalars and weighted sums for assign Oct 13, 2022
@connorjward connorjward marked this pull request as ready for review October 13, 2022 11:50
@connorjward connorjward force-pushed the connorjward/assign-weighted-sums-only branch 2 times, most recently from 2929450 to a0779d1 Compare October 13, 2022 14:10
@connorjward connorjward mentioned this pull request Oct 26, 2022
Merged
JDBetteridge
JDBetteridge requested changes Oct 26, 2022
View reviewed changes
Copy link
Member

@JDBetteridge JDBetteridge left a comment

Choose a reason for hiding this comment

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

I think we are mainly just waiting on some things to be fixed for adjoint

.github/workflows/build.yml Outdated Show resolved Hide resolved
firedrake/assemble.py Outdated Show resolved Hide resolved
firedrake/assign.py Outdated Show resolved Hide resolved
firedrake/function.py Show resolved Hide resolved
firedrake/utils.py Show resolved Hide resolved
dham
dham requested changes Nov 2, 2022
View reviewed changes
firedrake/assign.py Show resolved Hide resolved
@connorjward connorjward force-pushed the connorjward/assign-weighted-sums-only branch 3 times, most recently from ec85c45 to c1736f4 Compare November 11, 2022 23:02
@connorjward connorjward force-pushed the connorjward/assign-weighted-sums-only branch from c1736f4 to 4e50335 Compare November 16, 2022 16:22
JDBetteridge
JDBetteridge previously approved these changes Nov 16, 2022
View reviewed changes
dham
dham previously approved these changes Nov 16, 2022
View reviewed changes
ReubenHill
ReubenHill requested changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@ReubenHill ReubenHill left a comment

Choose a reason for hiding this comment

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

Please check that docs are up to date

@ReubenHill
Copy link
Contributor

Please check that docs are up to date

Poke me to approve when done

@connorjward
Rewrite assign
7e13000 
This commit:
- Restricts `assign` to only work for weighted sums of coefficients
  (plus addition of constants).
- Expunges codegen in favour of directly manipulating numpy arrays.
- Introduces an `Assigner` class to speed up repeated `assign` calls.
@connorjward
Only call continue_annotation once in test so stop_annotating works a…
61cb5f2 
…s expected
@connorjward
Raise exception if taping is on inside FormAssembler.assemble
ad6d7c8
@connorjward connorjward dismissed stale reviews from dham and JDBetteridge via ad6d7c8 November 17, 2022 15:49
@connorjward connorjward force-pushed the connorjward/assign-weighted-sums-only branch from 4e50335 to ad6d7c8 Compare November 17, 2022 15:49
@connorjward
Copy link
Contributor Author

@ReubenHill I think the docs all look fine. This is ready to go.

@ReubenHill ReubenHill merged commit 2ec31f9 into master Nov 18, 2022
@ReubenHill ReubenHill deleted the connorjward/assign-weighted-sums-only branch November 18, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wence- wence- wence- left review comments

@ReubenHill ReubenHill ReubenHill approved these changes

@JDBetteridge JDBetteridge JDBetteridge left review comments

@dham dham dham left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@connorjward @ReubenHill @dham @wence- @JDBetteridge