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

Compatibility update for dolfinx 0.9.0 and preCICE v3 #28

Merged
merged 54 commits into from
Mar 11, 2025

Conversation

NiklasVin
Copy link
Collaborator

@NiklasVin NiklasVin commented Dec 12, 2024

Updates the fenicsx-adapter such that it is compatible with dolfinx 0.8.0 0.9.0 and preCICE v3.
The structure of the current state stays more or less the same but I am planning to add ideas and features from #26.
The changes are also resolving #17.
The changes also include the issues addressed in #13. Unfortunately, I saw this PR too late and implemented more or less exactly the same

Todos

@NiklasVin NiklasVin changed the title Compatibility update for dolfinx 0.8.0 and preCICE v3 Compatibility update for dolfinx 0.9.0 and preCICE v3 Jan 2, 2025
…d it seems like the coupling boundary is the main issue for the errors but it converges and looks good otherwise
@BenjaminRodenberg
Copy link
Member

Thank you for this update!

I changed 0.8.0 in the description to 0.9.0 since you also mention 0.9.0 in the title of this PR.

I will close #13 now assuming that the PR is outdated and superseeded by this one. I think any valuable changes in #13 should be moved over here if possible.

Please also update the README and mention FEniCSx Version 0.9.0 as dependency.

@BenjaminRodenberg
Copy link
Member

BenjaminRodenberg commented Feb 17, 2025

We should merge #26 such that the contribution of @efirvida shows up in the history. I see the danger that we move ideas from #26 into this PR and then #26 will never be merged into the history.

I don't have a very good overview here but I see #28 as a maintainment PR for compatibility with FEniCSx 0.9.0 (also updating the partitioned heat equation tutorial) and #26 as a feature PR (adding the perpendicular flap).

I see two options (depends on how crucial the ideas contributed by #26 for #28):

  1. First merge Adapter precice 3.1 with 3D support and "point load" #26 then Compatibility update for dolfinx 0.9.0 and preCICE v3 #28: If merging Adapter precice 3.1 with 3D support and "point load" #26 into develop leads to a non-working state I don't see this as a big problem as long as merging Compatibility update for dolfinx 0.9.0 and preCICE v3 #28 will fix this situation.
  2. First merge Compatibility update for dolfinx 0.9.0 and preCICE v3 #28 then Adapter precice 3.1 with 3D support and "point load" #26: If Compatibility update for dolfinx 0.9.0 and preCICE v3 #28 is close to a working state that we can merge we can merge Compatibility update for dolfinx 0.9.0 and preCICE v3 #28 without adding any ideas from Adapter precice 3.1 with 3D support and "point load" #26 here. We could integrate the features provided via Adapter precice 3.1 with 3D support and "point load" #26 at a later point in time.

Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the tests and CI to work with this PR. Also added some minor comments. This is not a complete review.

I think it's already good news that the partitioned heat conduction test in the CI works 🎉

edit: at least it worked for a2c6c85. Not sure why I does not work anymore since the changes are unrelated.

@NiklasVin NiklasVin marked this pull request as ready for review March 8, 2025 17:34
* rename tutorials/fenicsx to tutorials/solver-fenicsx
* only use a single requirements.txt
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the tutorial and looks good. There are still some problems with errors and iteration count but let's deal with this at a later point in time.

I also fixed some tests.

Note that running the tutorial does not work out of the box since you need to setup the venv manually (remove fenicsxprecice from requirements.txt and install it with pip3 install ../../../.). But I also do not think we need this since the tutorial will be moved somewhere else at a later point in time.

@BenjaminRodenberg
Copy link
Member

What worries me a bit: From my perspective bf5eda2 is necessary that the tests pass. At least locally I get errors if I do not apply this fix. However, the CI passes apparently for c40fb25. We should investigate this more closely in the future since this means that our tests do not fail as expected.

Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the adapter code changes, and they look reasonable 👍 I did not test the tutorial case as others have already done so. Do we want to merge the partitioned-heat-conduction case files?

@BenjaminRodenberg
Copy link
Member

Do we want to merge the partitioned-heat-conduction case files?

Definitely! The tutorial structure also needs an update. The diff is a bit hard to read but I checked it again and there are no files that have been added accidentally.

I will merge this PR now.

@BenjaminRodenberg BenjaminRodenberg merged commit 5d8b553 into precice:develop Mar 11, 2025
6 checks passed
@BenjaminRodenberg BenjaminRodenberg added this to the v0.1.0 milestone Mar 12, 2025
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.

4 participants