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

nutils additions for the perpendicular flap example #429

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from

Conversation

gertjanvanzwieten
Copy link
Collaborator

@gertjanvanzwieten gertjanvanzwieten commented Jan 4, 2024

This PR reworks the Nutils fluid participant to make the code cleaner and compatible with Nutils 8, while remaining compatible with Nutils 7. It also adds a solid participant compatible only with Nutils 8. Only the final commit relates to the solid participant so it can trivially be split off into a separate PR if that is preferred.

The fluid modifications are organized in many small commits with their distinct purpose hopefully clear from the commit message. Up to d89f08d the formulation itself is not modified, though results change slightly because of minor bug fixes (e.g. 4b7b925, 6e05ed0). Beyond that the formulation is progressively simplified to the point of requiring only the previous solution, rather than the past four. The change of d89f08d could optionally be modified to involve the previous F for higher accuracy, though at the expense of oscillations, which the current implementation also suffers from -- the proposed formulation is oscillation free. Commit 35b8b34 does away with the hacky qw definition by switching to a uniform integration scheme, which anyway seems like reasonable enough given the non-polynomial nature of the integrand. All in all, the modified script is of a more standard nature and should be easier to follow for most Nutils users.

The failing Lint docs test seems unrelated to the changes in this PR.

closes #217

@gertjanvanzwieten gertjanvanzwieten marked this pull request as ready for review January 9, 2024 10:01
@uekerman
Copy link
Member

uekerman commented Jan 9, 2024

Thanks for the nice modification and documentation 🙏

I would indeed suggest to split the PR into two. The fluid modifications and the new solid participant. The latter should be quick to merge. The fluid modifications are a bit tricky. Nice job organizing the changes in simple commits. This way it should be easy to pick.

Beyond that the formulation is progressively simplified to the point of requiring only the previous solution, rather than the past four.

The old formulation was complicated indeed. We created it when we wanted to test the waveform relaxation in preCICE. For this, we needed a clean second-order fluid participant, including the mesh movement and the force computation. Back then, I experimented a lot and could not find a simpler solution. I did not yet look at all details, but I am afraid that the new formulation is no longer second order in time.

@gertjanvanzwieten Could you please remove the last commit here and move it to a new PR?

@gertjanvanzwieten
Copy link
Collaborator Author

I moved the last commit to #431, but made it a draft for reasons explained there.

As for the formulation, I wasn't sure whether the formulation was set in stone, hence the commit layout as it is. Naturally these are all just suggestions.

That said, the solution doesn't seem change much looking at plot-displacement.sh, and if it does then there is still this idea with involving the former F that might restore 2nd order for the traction evaluation to experiment with. This would involve changing line 72 to:

resF = res + (couplinginterface.sample('gauss', 4).integral('ubasis_ni F_i d:x' @ ns))

Note the added . It does induce oscillations which is why I left it out, but code wise it's trivial. For the formulation itself I'm not certain that this is not still 2nd order.

@IshaanDesai IshaanDesai changed the base branch from master to develop January 10, 2024 06:17
@fsimonis fsimonis mentioned this pull request Jan 18, 2024
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.

2 participants