FluidityProject / fluidity

Fluidity
http://fluidity-project.org
Other
366 stars 115 forks source link

Adding subroutines to zero the bubble dof in fields of different rank… #277

Closed stephankramer closed 4 years ago

stephankramer commented 4 years ago

… and doing this for rotational null spaces.

Rotational null spaces on bubble function spaces previously just evaluated a rotational component at the cell center for the bubble. This isn't actually a representation of a rotation anymore. The dof associated with the bubble should be zero in this case so this commit does just that.

Currently the subroutines for zeroing the bubble just assume that the final dof in an element is the bubble. Can't see any better logic than this but if element numbering ever changes this would have to change too.

stephankramer commented 4 years ago

I think we had some interesting discussion on this, but I can't quite remember - oh, was it that we were saying that the underlying issue is that for the bubble element the non-bubble basis functions should be modified so that they are zero in the bubble location? I think that would avoid the issue altogether, but there might be other consequences... What actually happens when we prescribe a bubble function? Cause that would have the same issue, no? In any case, we can revisit later, I don't think we're that invested in bubbles anyway - happy for this to go in...

cianwilson commented 4 years ago

Yeah, I think the issues were bigger picture...

This was just necessary to get the p2bp1dg cases working. Did those get added as tests in some branch?

stephankramer commented 4 years ago

Yes, the only reason I put this up for PR now is because this branch has been merged into analytical-benchrmarks-remeaned which has bubble versions of the benchmarks. We probably need to do a cull of the number of tests in that branch, but since the point of this exercise is getting the functionality of the paper in master (and we do present bubble results), I think we do need this branch (and at least one of the corresponding tests).

I think you are right: this should also be applied to translational null spaces. For a function on a bubble-element mesh, if you set all nodes to the same value, you do not get a constant field: instead you get the constant field plus a bubble inside each element, so that the interpolated value in the bubble node is twice the constant value. And indeed this isn't tested by the analytical benchmarks...

So the options are:

cianwilson commented 4 years ago

Couple of thoughts: In the analytical-benchrmarks-remeaned branch are both this patch and the remove L2 (#278) patch applied? I don't see anything in the big L2 removal that would zero out the bubble dof and yet, presumably, the p2bp1dg free slip case passes. Any idea why that is? I'm just trying to think of other places the bubble dof should be zeroed.

If we do want to zero the bubble for the translational null spaces we could just do a petsc is this a null space test rather than anything as complicated as the cylindrical cases.

stephankramer commented 4 years ago

In the analytical-benchrmarks-remeaned branch are both this patch and the remove L2 (#278) patch applied?

yes

I don't see anything in the big L2 removal that would zero out the bubble dof

indeed, and I think you're right that it should. I'll add it and rerun the bubble cases and see if the answer changes...

stephankramer commented 4 years ago

Right, so I've added the zero bubble to the big-L2 removal step as well (after merging master (which now contains big-L2) into this branch. The difference for the benchmark tests is minimal, something like a 1e-3 change in the order of convergence. The reason is, I think, that the big-L2 removal has minimal effect on these benchmarks in general: the meshes are very uniform. The only reason we added it was for consistency. I think started noticing we needed it on adaptive cases only.

I wasted a few hours looking at fixing this for translational nullspaces - after realizing that even putting an error message in would be tricky. I've put in a fix: I tried to make a p2bp1dg case of Stokes_mms_cg_p2p1_periodic. The fix, as expected, changes the petsc nullspace test from not passing to passing, but the case does not converge to steady state and I frankly have given up caring. So I suggest we simply leave it at this. I think the current implementation of bubbles in fluidity just does not fit very well and I don't think we really have a purpose for it at the moment.

cianwilson commented 4 years ago

Yes, you're right. Bubbles are generally broken unless used very carefully.

Did you commit Stokes_mms_cg_p2p1_periodic somewhere? Was going to take a look quickly but probably shouldn't waste my time.

Do we need to get @drhodrid to review this?