Force a single-value per element for elementwise reductions #195
Conversation
|
@inducer CI is now failing for some mysterious reason ever since #200. Here's a link to the log: https://github.com/inducer/grudge/runs/4638364194?check_suite_focus=true#step:3:606. It's not clear why this (and all other PRs) are experiencing problems related to this. |
|
There were a few legitimate CI failures yesterday (which I fixed), but these here look spurious. I've rerun the checks. |
There was a problem hiding this comment.
Thanks! The main piece of this LGTM, however I have some questions about the area estimates in the time step estimation. If you'd rather deal with those separately (by filing an issue), I can live with that. I do think it's worth talking about this code briefly now, though.
grudge/dt_utils.py
Outdated
| 1 if actx.supports_nonscalar_broadcasting | ||
| else afgrp.nunit_dofs |
There was a problem hiding this comment.
This nodal averaging is super sketchy to me. Why not just use honest quadrature (using afgrp.weights or some such) to compute areas?
There was a problem hiding this comment.
@inducer I agree this is a bit ugly, but let me explain why this was done.
You recall the issues with nonscalar broadcasting for array contexts right? Well when an array context can't support it, you get repeated values at each node, rather than the single DOFArray with one value per cell. This tried to address this difference.
Why not just use honest quadrature (using afgrp.weights or some such) to compute areas?
Um, that's what is happening if you look up a few more lines. In the definition of surface_areas:
Line 274 in be1e724
There was a problem hiding this comment.
Ah. Missed that. Sorry! In this case, I would restate my comment here to maybe make two separate code paths: one for "capable" actxs, implementing just the sum over faces (and likely reshaping away the 1-long axis), and another for the "less capable" ones. That way, the math intent is easier to recognize from the (shorter, simpler, more sensible) code, but will still execute with the old and busted foundation. Does that sound reasonable?
There was a problem hiding this comment.
Sounds good! Yeah I like that, I'll update accordingly
There was a problem hiding this comment.
Updated: 4f04de2
grudge/dt_utils.py
Outdated
| 1 if actx.supports_nonscalar_broadcasting | ||
| else afgrp.nunit_dofs |
There was a problem hiding this comment.
Ah. Missed that. Sorry! In this case, I would restate my comment here to maybe make two separate code paths: one for "capable" actxs, implementing just the sum over faces (and likely reshaping away the 1-long axis), and another for the "less capable" ones. That way, the math intent is easier to recognize from the (shorter, simpler, more sensible) code, but will still execute with the old and busted foundation. Does that sound reasonable?
Co-authored-by: Kaushik Kulkarni <15399010+kaushikcfd@users.noreply.github.com>
thomasgibson
force-pushed
the
thg/reductions-update
branch
from
6c405cc
to
e9a1d6c
Compare
This PR fixes #184
The logic here is that it's safe to force the elementwise reductions to have a shape of
(nelem, 1)by definition of a reduction in this context; no need to broadcast to a full DOFArray