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.

Sign up for GitHub

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

Jump to bottom

Compatibility update for v3.0.0 #153

Merged
merged 33 commits into from
Sep 28, 2023
Merged

Compatibility update for v3.0.0 #153

merged 33 commits into from
Sep 28, 2023

Conversation

BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Jul 12, 2022 •
edited by IshaanDesai
Loading

Ensures compatibility to precice:develop.

@BenjaminRodenberg BenjaminRodenberg added the breaking change Breaking change that should only be included in a major release label Jul 12, 2022
@BenjaminRodenberg BenjaminRodenberg self-assigned this Jul 12, 2022
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Jul 12, 2022
Merged
@MakisH MakisH mentioned this pull request Nov 24, 2022
Closed
@IshaanDesai IshaanDesai mentioned this pull request Mar 28, 2023
Closed
BenjaminRodenberg
BenjaminRodenberg commented Mar 28, 2023
View reviewed changes
Copy link
Member Author

@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.

Since the test with the partitioned heat tutorial is failing: I guess we also need to update the tutorials. Into the rabbit hole 🐇

fenicsprecice/expression_core.py Show resolved Hide resolved
fenicsprecice/fenicsprecice.py Show resolved Hide resolved
@BenjaminRodenberg BenjaminRodenberg added this to the v2.0.0 milestone Mar 29, 2023
@BenjaminRodenberg
Copy link
Member Author

I also added the v2.0.0 milestone here, because merging this PR would be breaking due to the changes in the action interface. We could also design this in a non-breaking fashion and hide the breaking changes of preCICE v3 inside of the adapter, but this might be more confusing than useful, I guess.

@IshaanDesai IshaanDesai mentioned this pull request Mar 29, 2023
Merged
@BenjaminRodenberg
Copy link
Member Author

Since this PR is already quite old: At the moment it is not on the first place of my list of priorities, but I expect to work with preCICE v3 + FEniCS within the next few months again. We can wait with this issue until then and I will occasionally review and merge. If something else pops up in the meantime, we can, of course, take care of this issue earlier.

@IshaanDesai
Copy link
Member

Since this PR is already quite old: At the moment it is not on the first place of my list of priorities, but I expect to work with preCICE v3 + FEniCS within the next few months again. We can wait with this issue until then and I will occasionally review and merge. If something else pops up in the meantime, we can, of course, take care of this issue earlier.

I plan to work on this towards the end of this month. Most of the updates in the adapter itself have been done. The major work is actually in updating the tutorials in precice/tutorials#333.

BenjaminRodenberg added a commit to precice/tutorials that referenced this pull request Jul 11, 2023
@BenjaminRodenberg
Synchronize partitioned-heat-conduction with v3 of fenics-adapter.
3a60c6a 
* Synchronize with 748c934 from precice/fenics-adapter#153
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Jul 12, 2023
Open
@BenjaminRodenberg
Copy link
Member Author

@IshaanDesai I'll try to wrap this PR up soon. Main problem here are the unit tests. I assume the tutorial tests should work as soon as precice/tutorials#333 is merged.

BenjaminRodenberg
BenjaminRodenberg commented Sep 28, 2023
View reviewed changes
fenicsprecice/adapter_core.py Outdated
v1, v2 = vertices(e1)
_, v3 = vertices(e2)
assert (v3 != v1)
assert (v2 != v2)
Copy link
Member Author

Choose a reason for hiding this comment

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

I found this problem by running channel-transport-reaction. But even fixing this obviously wrong assertion does not help. There seems to be some issue with the function get_coupling_triangles. We obviously need a test for this function and need to fix it. @IshaanDesai can you take a look?

@BenjaminRodenberg BenjaminRodenberg merged commit 8a21819 into develop Sep 28, 2023
7 checks passed
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Feb 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IshaanDesai IshaanDesai IshaanDesai approved these changes

Assignees

@IshaanDesai IshaanDesai

Labels
breaking change Breaking change that should only be included in a major release
Projects
preCICE version 3 overview
Status: Done
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@BenjaminRodenberg @IshaanDesai