search

mesh-adaptation / animate

Anisotropic mesh adaptation toolkit for Firedrake
MIT License
5 stars 0 forks source link

Mass lumping: vector fields #121

Closed ddundo closed 4 months ago

ddundo commented 4 months ago

Hi @jwallwork23, I have been trying again to get this to work for vector fields and I think I got it. I hope I won't sound rude when I ask this: could I please ask you if you could take a look into this, instead of prioritising https://github.com/mesh-adaptation/goalie/issues/188 as you said you would? Lumping really did make a significant difference in my case, whereas the adjoint action issue is (I assume, but could very easily be wrong) not so significant. So I'd really like to get this sorted out :) and I hope this will be pretty straightforward.

I basically just put what you did here inside a for loop, but I had to create FunctionSpaces for individual components in a messy way, since just doing source.sub(i).function_space() didn't work. Anyway, the ping pong demo results (i.e. scalar fields) are identical, and I tested it on a vector field and it looks good.

I also tried it out on the burgers.py demo, by passing transfer_kwargs={"bounded": True} to MeshSeq and then got this (tolerance of 1e-7). In doing that, I also had to do pyadjoint.pause_annotation() inside solve_forward before transferring the checkpoint to the next subinterval (and then I continued annotating after the transfer). Otherwise the assign in source_sub = ffunc.Function(Vs_sub).assign(source.sub(i)) raises an error.

image

ddundo commented 4 months ago

Sorry about the long tests... I forgot I modified the tolerance and went to sleep after pushing this.

ddundo commented 4 months ago

After running my test cases again, I again got slightly worse results with this "minimally diffusive" projection than with the previous lumped-only option that was removed in the original PR. Not sure why, but anyway it's likely I did something wrong here. But, I already get very very good results with the lumping-only option, so even if this would be better it would be marginal. And it wouldn't justify the added cost of these post-processing operations.

So I think it's best that I close this and not delay the paper because of it. Thanks nonetheless @jwallwork23! But in the meantime, could you please add back in the lumped-only option in the other branch (i.e. without the postprocessing step)?

ddundo commented 2 months ago

@acse-ej321 I lost access to the Teams chat since I'm external, so I'm tagging you here as well :)