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

Mass lumping: vector fields #121

Closed
wants to merge 2 commits into from
Closed

Conversation

ddundo
Copy link
Member

@ddundo ddundo commented May 12, 2024

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 mesh-adaptation/goalie#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
Copy link
Member Author

ddundo commented May 12, 2024

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

@ddundo
Copy link
Member Author

ddundo commented May 12, 2024

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 ddundo closed this May 12, 2024
@ddundo
Copy link
Member Author

ddundo commented Jul 11, 2024

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

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.

1 participant