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

Support averaging operators (fixes #107) #158

Merged
merged 6 commits into from
Mar 24, 2018
Merged

Support averaging operators (fixes #107) #158

merged 6 commits into from
Mar 24, 2018

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Feb 14, 2018

Done by just integrating the averaged expression over the relevant
entity.

Needs a matching UFL merge.

@wence-
Copy link
Contributor Author

wence- commented Feb 14, 2018

@wence-
Copy link
Contributor Author

wence- commented Feb 14, 2018

This does the wrong thing for cell_avg(inner(f, f))*dx where f is VFS right now. I think I'm just being dense.

Compute the volume by summing the quadrature weights.
Done by just integrating the averaged expression over the relevant
entity.
@wence-
Copy link
Contributor Author

wence- commented Feb 14, 2018

Think I fixed it, the XXX average is (sum_q w_q expr_q) / (reference_entity_volume), not just sum_q w_q expr_q.

@blechta
Copy link
Contributor

blechta commented Feb 14, 2018

For affine-transformed cell, yes. For bendy cells each infinitesimal reference cell volume contributes differently to physical volume... Not sure which case is Q1 cell.

@wence-
Copy link
Contributor Author

wence- commented Feb 15, 2018

I don't think so. Here's my reasoning. For a function f, the mean over some compact domain U is 1/Vol(U) \int f dU. Which, with quadrature, is (\sum_q w_q f_q) / (\sum_q w_q).

Now consider integrating cell_avg(f)*dx.

This is \int_\Omega \bar{f} dx = \sum_e \int_e \bar{f} dx_e.

Now I pull back to reference space:

\sum_e \int_e \bar{f} dx_e = \sum_e \int_E \bar{\tilde{f}} \det J dX

and \bar{\tilde{f}} = \int_E \tilde{f} dX / Vol(dX) (because we're already in reference space).

I think the only thing I assumed here was that averaging commutes will pullback.

Does this make sense?

@wence-
Copy link
Contributor Author

wence- commented Feb 20, 2018

This is still totally borked. Promotes bugs in tsfc optimisation passes (possibly I'm doing something wrong). Or doesn't work at all.

@blechta
Copy link
Contributor

blechta commented Feb 20, 2018

I don't think that the assumption is correct. We have

\bar{f} = \frac{ \int_e f dx }{ \int_e dx } = \frac{ \int_E \hat{f} |\det J| dX }{ \int_E |\det J| dX }

This is clearly not equal to

\frac{ \int_E \hat{f} dX }{ \int_E dX }

unless \det J is constant in E.

@wence-
Copy link
Contributor Author

wence- commented Feb 20, 2018

Thanks. I've come to that conclusion too. The latest commits do this "right", but still have issues elsewise. So I'm holding off any merging for now.

@miklos1
Copy link
Member

miklos1 commented Feb 23, 2018

I like that you could implement this in such a small diff.

@wence- wence- merged commit f9ffdf8 into master Mar 24, 2018
wence- added a commit that referenced this pull request Mar 24, 2018
* avg-not-terminal:
  Pass correct argument multiindices
  Nearly done.
  WIP: and facet_avg in the same way
  WIP: do cell_avg properly
  Support averaging operators (fixes #107)
  Support ReferenceFacetVolume on non-simplex cells
@wence- wence- deleted the avg-not-terminal branch May 3, 2018 09:36
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