Skip to content

Conversation

@BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Dec 7, 2020

This PR ensures compatibility of the tutorials with v1.0.0 of the fenics-adapter (see https://github.com/precice/fenics-adapter/milestone/4).

I've collected the following relevant commits from https://github.com/precice/tutorials/tree/develop (I'll provide the commit hashes of the original commits below).

We can still change the content of this PR and the list of commits below. I would like to maintain the history for this PR, if we merge it to master. I used the following strategy. If we decide that this is useful, we should document it at a proper place in the future:

  1. git checkout develop
  2. git checkout -b fenics-adapter-v1.0.0
  3. git rebase -i eb3283ca64932fb2d674bed1b8b677528d5c1b76, use commits from below for rebase. (eb3283c is the last commit that we can directly apply to master.)
  4. git push to origin/fenics-adapter-v1.0.0
  5. after merging this PR origin/master and origin/fenics-adapter-v1.0.0 are even
  6. git checkout master
  7. git pull
  8. git checkout develop
  9. git merge master (might be a better idea to rebase here to avoid duplicates)
  10. git push to origin/develop (then we will have to git push --force to develop)

Required commits:

# pick 0382555 Update case according to https://github.com/precice/fenics-adapter/pull/81
# pick c764405 Update get_point_sources
# pick a178d1c Update get_point_sources
# pick 7d4eb3c Revert "Update get_point_sources"
# pick 407a235 Uses new SegregatedRBFInterpolationExpression from https://github.com/precice/fenics-adapter/pull/83
pick 88d2db5 Add valid interpolation strategy.
pick 475e406 Use rbf_segregated
pick c9d7408 Removes interpolation_type from all fenics adapter config files.
pick 7b74954 Fix output frequency of Solid
pick 8a51b25 Make output routine with dt_out more robust
pick ece21a0 Use same method for flux computation like in HT example. (#114)
pick f33edb6 Rename fenicsadapter to fenicsprecice (#113)
pick c20e1c3 Convert dt to float (#115)
pick 41b1a46 Initial change to FEniCS-Adapter initialization
pick ca1b1dd Merging write_function_space and write_function into write_object
pick 42bd9c5 Explicitly mentioning optional parameter names in adapter initialization
pick 860642a Adding comment to explain why a function space is passed twice to initialization
pick fdbee1b Initialize with V_g.sub(1) as write_object (#127)
pick bb5d51b Add SU2-FEniCS Perpendicular Flap FSI Tutorial (#91)
pick 1c891a7 Modify FEniCS tutorials to enable parallel runs (#120)
pick 107c076 Move */OpenFOAM-FEniCS to *_2D/OpenFOAM-FEniCS. (#141)
pick 595409d Ensuring consistency in FEniCS scripts for FSI tutorial cases  (#142)
pick ae77ae5 Fix ill-posed problem by providing additional data. (#144)

Shortcomings

If there is a better strategy, feedback would be very welcome. A big downside of the current strategy is that for each commit a new hash is generated. This means that as soon as we merge develop into master (i.e. next regular release), we will duplicate all commits from this PR. We can already see this in the log of develop after the last step described above:

commit f9c16f74955657593c2c8917e9e30e83c4919596 (HEAD -> develop)
Merge: 1c891a7 a1ca149
Author: BenjaminRueth <benjamin.rueth@tum.de>
Date:   Sun Dec 20 15:47:01 2020 +0100

    Merge branch 'master' into develop

commit a1ca14935f39fc22ffe33ed0dc877fd9422a713c (origin/fenics-adapter-v1.0.0, master, fenics-adapter-v1.0.0)
Author: Ishaan Desai <44898158+IshaanDesai@users.noreply.github.com>
Date:   Sat Dec 19 18:15:54 2020 +0100

    Modify FEniCS tutorials to enable parallel runs (#120)
    
    * Modifying all FEniCS based tutorials to be compatible with parallel design of FEniCS-Adapter
    Co-authored-by: BenjaminRueth <benjamin.rueth@tum.de>

commit 55232e384bd8dc5c48dc68834dc03b0d3527f7af
Author: Ishaan Desai <44898158+IshaanDesai@users.noreply.github.com>
Date:   Sat Dec 19 17:47:24 2020 +0100

    Add SU2-FEniCS Perpendicular Flap FSI Tutorial (#91)
    
    * Adding 2D perpendicular flap FSI tutorial with SU2-FEniCS
    Co-authored-by: Benjamin Rüth <benjamin.rueth@tum.de>

commit c410372a24f7c2316b533fb90392ee66426c34ae
Author: Benjamin Rüth <benjamin.rueth@tum.de>
Date:   Thu Dec 10 17:19:03 2020 +0100

    Initialize with V_g.sub(1) as write_object (#127)

commit 1c891a7882ddf3a2abdfa50ecafb941f1395eebb (origin/develop)
Author: Ishaan Desai <44898158+IshaanDesai@users.noreply.github.com>
Date:   Sat Dec 19 18:15:54 2020 +0100

    Modify FEniCS tutorials to enable parallel runs (#120)
    
    * Modifying all FEniCS based tutorials to be compatible with parallel design of FEniCS-Adapter
    Co-authored-by: BenjaminRueth <benjamin.rueth@tum.de>

commit bb5d51b27be037f8a7e579a43ba878b7ece25bf6
Author: Ishaan Desai <44898158+IshaanDesai@users.noreply.github.com>
Date:   Sat Dec 19 17:47:24 2020 +0100

    Add SU2-FEniCS Perpendicular Flap FSI Tutorial (#91)
    
    * Adding 2D perpendicular flap FSI tutorial with SU2-FEniCS
    Co-authored-by: Benjamin Rüth <benjamin.rueth@tum.de>

...

commit 8cf97fda8a4a4d3c551fd4a970e3f939c7be6167
Author: Benjamin Rüth <benjamin.rueth@tum.de>
Date:   Sun Nov 8 16:06:49 2020 +0100

    Convert dt to float (#115)

...

commit c20e1c396c402b9ad5b7c6b581ffb45ef26a91e1
Author: Benjamin Rüth <benjamin.rueth@tum.de>
Date:   Sun Nov 8 16:06:49 2020 +0100

    Convert dt to float (#115)

@BenjaminRodenberg
Copy link
Member Author

BenjaminRodenberg commented Dec 7, 2020

I now decided to use git rebase -i instead of git cherry-pick. I removed all the commits that we do not need in this interactive session (see above). This keeps the original hashes of 0382555 and c764405. However, later commits will still get a new hash. edit: as soon as we merge this PR into master and then master into develop everything is clean.

@BenjaminRodenberg
Copy link
Member Author

What will happen with this PR in the future? We will first merge fenics-adapter-v1.0.0@3d87fe8 into master@95cc1ed. Then master@3d87fe8 into develop@0252594 to synchronize both branches. At some point in time in the future (regular release) we will merge develop@xxxxxxx (in example below this is the merge commit 9c13a46) into master@3d87fe8. I already followed this strategy locally and got the following output for the merge of develop into master:

benjamin@benjamin-ThinkPad-X1-Yoga-2nd:~/tutorials$ git merge develop 
Updating 3d87fe8..9c13a46
Fast-forward
 CHT/flow-over-plate/buoyantPimpleFoam-nutils/Nutils/cht.py                     |   2 +-
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/0.orig/U                            |  13 ++--
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/constant/dynamicMeshDict            |   0
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/blockMeshDict                |  34 +++++-----
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/blockMeshDict_double_refined | 346 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/blockMeshDict_refined        | 346 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/controlDict                  |  19 +++++-
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/decomposeParDict             |   4 +-
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/fvSchemes                    |  16 +----
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/fvSolution                   |  65 +++++++++++-------
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/README.md                                 |   2 +-
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Solid/linear_elasticity.prm               |   4 +-
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Solid/nonlinear_elasticity.prm            |   2 +-
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/plotDisplacement.sh                       |   9 +++
 FSI/cylinderFlap_2D/OpenFOAM-deal.II/precice-config.xml                        | 117 +++++++++++++++-----------------
 FSI/flap_perp/OpenFOAM-CalculiX/README.md                                      |   2 +-
 FSI/flap_perp/SU2-CalculiX/Allclean                                            |  54 +++++++++++++++
 FSI/flap_perp/SU2-CalculiX/Allrun                                              |  77 +++++++++++++++++++++
 FSI/flap_perp/SU2-CalculiX/Fluid/euler_config_coupled.cfg                      |  13 +++-
 FSI/flap_perp/SU2-CalculiX/Fluid/euler_config_coupled_mergeSolution.cfg        | 267 -----------------------------------------------------------------------
 FSI/flap_perp/SU2-CalculiX/README.md                                           |   8 ++-
 FSI/flap_perp/SU2-CalculiX/plotDisplacement.sh                                 |   2 +-
 FSI/flap_perp/SU2-CalculiX/precice-config.xml                                  |   2 +-
 FSI/flap_perp/SU2-CalculiX/runFluid                                            |  25 +++++++
 FSI/flap_perp/SU2-CalculiX/runSolid                                            |  14 ++++
 FSI/flap_perp/SU2-CalculiX/runTutorial_parallel.sh                             |  48 -------------
 FSI/flap_perp/SU2-CalculiX/runTutorial_serial.sh                               |  46 -------------
 27 files changed, 1033 insertions(+), 504 deletions(-)
 mode change 100755 => 100644 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/constant/dynamicMeshDict
 create mode 100644 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/blockMeshDict_double_refined
 create mode 100644 FSI/cylinderFlap_2D/OpenFOAM-deal.II/Fluid/system/blockMeshDict_refined
 create mode 100755 FSI/cylinderFlap_2D/OpenFOAM-deal.II/plotDisplacement.sh
 create mode 100755 FSI/flap_perp/SU2-CalculiX/Allclean
 create mode 100755 FSI/flap_perp/SU2-CalculiX/Allrun
 delete mode 100644 FSI/flap_perp/SU2-CalculiX/Fluid/euler_config_coupled_mergeSolution.cfg
 create mode 100755 FSI/flap_perp/SU2-CalculiX/runFluid
 create mode 100755 FSI/flap_perp/SU2-CalculiX/runSolid
 delete mode 100755 FSI/flap_perp/SU2-CalculiX/runTutorial_parallel.sh
 delete mode 100755 FSI/flap_perp/SU2-CalculiX/runTutorial_serial.sh

This looks good to me. Obviously all changes that affect FEniCS-bases tutorials have already been merged into master via #124. The only changes that are remaining are the ones not connected to FEniCS.

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.

Manual check of all tutorials is done. All good 👍

@IshaanDesai IshaanDesai marked this pull request as ready for review December 20, 2020 17:40
@BenjaminRodenberg
Copy link
Member Author

@MakisH , @fsimonis: Can you comment on this PR? Not really on the content, but more on the general git merging strategy? Please speak up, if you think everything is a horrible idea. I think we will have to be careful with the merge master -> develop technically we cannot do anything wrong with merging this PR (yet).

Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

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

I am not sure I follow everything, but I understand that you did several changes in your branch, and now you want to merge to master. For some reason I don't fully understand, we will get some duplicate (empty) commits when we merge from develop to master in the next release.

Here are a few thoughts:

  • All the changes here are related to FEniCS (good).
  • The history of the tutorials is already quite dirty. I would not worry too much, you already seem to have thought well of every step.
  • Since you are afraid that something go wrong when we merge develop to master, you can have the honor to do it yourself when it's time to release. 😉 I am optimistic that things will not go so bad. Worst case, we have to resolve some merge conflicts.
  • The most important aspect I see is to merge these changes soon and to then synchronize the restructuring branch soon afterwards.
  • Finally, I don't like the idea of directly merging to master right now. Do you have a good reason for this? Why skip develop? Why not merge to develop and then merge develop to master soon after? We don't need to wait for the next preCICE release to update the tutorials, develop still works perfectly fine with preCICE v2.1.1.

@BenjaminRodenberg
Copy link
Member Author

Thanks for the comment!

I am not sure I follow everything, but I understand that you did several changes in your branch, and now you want to merge to master. For some reason I don't fully understand, we will get some duplicate (empty) commits when we merge from develop to master in the next release.

Important information that is missing: This is not a feature branch, but one where I collected all the FEniCS-related changes from develop.

Here are a few thoughts:

  • All the changes here are related to FEniCS (good).

  • The history of the tutorials is already quite dirty. I would not worry too much, you already seem to have thought well of every step.

  • Since you are afraid that something go wrong when we merge develop to master, you can have the honor to do it yourself when it's time to release. wink I am optimistic that things will not go so bad. Worst case, we have to resolve some merge conflicts.

Sounds at least, like I did not commit any major git sins ;)

  • The most important aspect I see is to merge these changes soon and to then synchronize the restructuring branch soon afterwards.

Fully agree: I really would like to do a release before #125.

  • Finally, I don't like the idea of directly merging to master right now. Do you have a good reason for this?

Main reason: We want to release the fenics-adapter soon and therefore also have a corresponding release of the tutorials.

Why skip develop?

The changes from this branch are already part of develop.

Why not merge to develop and then merge develop to master soon after?

The main reason I'm not directly merging develop into master is since I don't want to risk breaking the other tutorials that are not related to FEniCS (i.e. Deal.II).

We don't need to wait for the next preCICE release to update the tutorials, develop still works perfectly fine with preCICE v2.1.1.

If the current state of develop is ready for releasing, we can also just do a "normal" release and close this PR. But then we will not have the "nice" FEniCS-only release ("All the changes here are related to FEniCS (good).")

@MakisH
Copy link
Member

MakisH commented Dec 21, 2020

Why not merge to develop and then merge develop to master soon after?

The main reason I'm not directly merging develop into master is since I don't want to risk breaking the other tutorials that are not related to FEniCS (i.e. Deal.II).

I don't see any such issue at the moment. I see more problematic bypassing our branching model (as you already pointed out that it may need some ninja tricks to not end up with spaghetti bolognese).

We don't need to wait for the next preCICE release to update the tutorials, develop still works perfectly fine with preCICE v2.1.1.

If the current state of develop is ready for releasing, we can also just do a "normal" release and close this PR. But then we will not have the "nice" FEniCS-only release ("All the changes here are related to FEniCS (good).")

You could do a similar thing targetting the develop branch next time (since now everything is already in develop). If you need it for future reference, you can always close this and the diff will remain (at least if the branch remains).

Other than being able to easily check what we needed to change, I don't see a reason to make it a "FEniCS-only release". The develop should always be releasable (more on this in the context of our versioning strategy).

@BenjaminRodenberg
Copy link
Member Author

Why not merge to develop and then merge develop to master soon after?

The main reason I'm not directly merging develop into master is since I don't want to risk breaking the other tutorials that are not related to FEniCS (i.e. Deal.II).

I don't see any such issue at the moment. I see more problematic bypassing our branching model (as you already pointed out that it may need some ninja tricks to not end up with spaghetti bolognese).

We don't need to wait for the next preCICE release to update the tutorials, develop still works perfectly fine with preCICE v2.1.1.

If the current state of develop is ready for releasing, we can also just do a "normal" release and close this PR. But then we will not have the "nice" FEniCS-only release ("All the changes here are related to FEniCS (good).")

You could do a similar thing targetting the develop branch next time (since now everything is already in develop). If you need it for future reference, you can always close this and the diff will remain (at least if the branch remains).

Other than being able to easily check what we needed to change, I don't see a reason to make it a "FEniCS-only release". The develop should always be releasable (more on this in the context of our versioning strategy).

Ok. Then I will close this PR and open a "real" release PR from develop to master. This avoids spaghetti bolognese and ninja tricks ;) If nobody stops me: As soon as the systemtests are good.

@uekerman
Copy link
Member

Other than being able to easily check what we needed to change, I don't see a reason to make it a "FEniCS-only release". The develop should always be releasable (more on this in the context of our versioning strategy).

But then we need to release everything else now as well? (all adapters) That's sth we wanted to avoid with the new release strategy, right? It needs to be possible to only release the FEniCS adapter.

BenjaminRodenberg added a commit that referenced this pull request Dec 23, 2020
* Applies suggestion from #124 (review)
* Add minor .gitignore fixes.
@BenjaminRodenberg
Copy link
Member Author

@IshaanDesai can you check again? I removed your previously pushed commit, since this a release branch. But actually something was wrong with my previous rebase. So good that you detected the error.

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.

Applying changes from: #142 will finalize the compatibility of tutorials with fenics-adapter v1.0.1

@BenjaminRodenberg
Copy link
Member Author

Just had a short concluding discussion with @IshaanDesai and @uekerman on this:

  • No push --force to develop ❗ Doing a push --force would allow us to keep master tidy (also in the future), but drive everybody using develop crazy, because commits on develop would be overwritten.
  • Do a merge of master into develop after the release. There should not be any merge conflicts, if everything is done correctly here. This will duplicate commits that were introduced in this PR via cherry-pick. However, this is the preferable way to mess up our history 😏

I will take care of the last few necessary synchronizations of this PR to develop and then merge into master as soon as precice/fenics-adapter#114 (v1.0.1) is released, if nobody protests.

BenjaminRodenberg added a commit that referenced this pull request Jan 8, 2021
* Applies suggestion from #124 (review)
* Add minor .gitignore fixes.
BenjaminRodenberg and others added 18 commits January 8, 2021 22:35
Output frequency of heat.py must match frequency of OpenFOAM Fluid case.
Compare writeInterval in controlDict (https://github.com/precice/openfoam-adapter/blob/develop/tutorials/CHT/flow-over-plate/buoyantPimpleFoam-laplacianFoam/Fluid/system/controlDict).
Ensures compatibility with changes introduced in precice/fenics-adapter@fef80b7
* Adding 2D perpendicular flap FSI tutorial with SU2-FEniCS
Co-authored-by: Benjamin Rüth <benjamin.rueth@tum.de>
* Modifying all FEniCS based tutorials to be compatible with parallel design of FEniCS-Adapter
Co-authored-by: BenjaminRueth <benjamin.rueth@tum.de>
* Applies suggestion from #124 (review)
* Add minor .gitignore fixes.
* Making SU2- and OpenFOAM- case of perp-flap for FEniCS consistent
@BenjaminRodenberg BenjaminRodenberg merged commit 5f4031f into master Jan 10, 2021
@BenjaminRodenberg BenjaminRodenberg deleted the fenics-adapter-v1.0.0 branch January 10, 2021 20:27
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.

5 participants