Skip to content

New cleaning tools#169

Merged
MakisH merged 12 commits intodevelopfrom
cleaning
Mar 25, 2021
Merged

New cleaning tools#169
MakisH merged 12 commits intodevelopfrom
cleaning

Conversation

@MakisH
Copy link
Copy Markdown
Member

@MakisH MakisH commented Mar 24, 2021

Closes #133.

Main files:

  • tools/cleaning-tools.sh: Contains one function for each solver (e.g. clean_openfoam), one shared function to clean the preCICE log files, and one function clean_tutorial.
  • <tutorial>/<case>/clean.sh: Only triggers the right clean_<solver>.
  • <tutorial>/clean-tutorial.sh: The same for every tutorial, only triggers clean_tutorial.
  • clean-all.sh: Triggers all clean-tutorial.sh, cleaning everything:
~/github/tutorials [cleaning]$ ./clean-all.sh 
- Cleaning up all tutorials...
-- Cleaning up all cases in /home/makish/github/tutorials/elastic-tube-3d...
--- Cleaning up OpenFOAM case in /home/makish/github/tutorials/elastic-tube-3d/fluid-openfoam
Cleaning case /home/makish/github/tutorials/elastic-tube-3d/fluid-openfoam
---- Cleaning up preCICE logs in /home/makish/github/tutorials/elastic-tube-3d/fluid-openfoam
--- Cleaning up CalculiX case in /home/makish/github/tutorials/elastic-tube-3d/solid-calculix
---- Cleaning up preCICE logs in /home/makish/github/tutorials/elastic-tube-3d/solid-calculix
-- Cleaning up all cases in /home/makish/github/tutorials/flow-over-heated-plate-nearest-projection...
--- Cleaning up OpenFOAM case in /home/makish/github/tutorials/flow-over-heated-plate-nearest-projection/fluid-openfoam
...

This design allows to clean the files at any of the three levels, with the assumption "from this level and deeper".

Maintainability is tremendously improved, as now most changes can happen in one place. The rest should be short and static enough (but also trivial to update, as the files are exactly the same). I could even symlink directly to one file in the tools/ (trying to think of counter-arguments).

Technical notes and design choices

  • All cleaning scripts are portable POSIX-compatible shell scripts (i.e. #!/bin/sh), without any Bash-specific feature.
  • I have checked them with shellcheck for potential issues.
  • Every script starts with set -e -u. These will make the script exit on errors and on undefined variables.
  • The scripts cannot be started from arbitrary directories (I may implement this, but I don't see much added value). The check we had before (cd ${0%/*} || exit 1) is not correct.
  • clean_precice_logs() lists all types of files explicitly. This should be a sanity check whenever we add new output file types.
  • Every clean_<solver> executes inside a subshell (( do_stuff )). This makes it a bit more state-independent ("do I need to cd back at the end?").
  • The number of dashes in the beginning of each line (-, --, ...) signifies the level and they serve as separators. Normally, there should be additional output from each file that is removed. The almost duplicate OpenFOAM output comes from the cleanCase of OpenFOAM.
  • In most (all?) OpenFOAM cases we have a first step where we copy the backup 0.orig/ to 0/. This is normally not needed, so I left this deletion step out of the function and added it explicitly to each case.
  • A few more files are intentionally cleaned up now:
    • The .foam files are now deleted and not recreated. This should be done by the run.sh (there was some motivation for this in the past, but it is not really relevant anymore). (update: done)
    • The elastic-tube-3d previously skipped deleting the OpenFOAM mesh, probably to save time. This was always confusing. I now delete them as well and I will add them to the .gitignore later. (update: reverted the change, handling similar to 0.orig in run.sh)
  • There is no participant information used anywhere, but this should be safe enough.

Still open

  • Check if the CalculiX, deal.II, Nutils, SU2, and code_aster are cleaned up completely (I just distilled the previous lists for now).
  • Add the missing clean.sh and clean-tutorial.sh to the not yet restructured cases and remove the exclusion from clean-all.sh. But that's probably a future todo.

This is part of a trilogy, with the next steps being a common .gitignore (#87) and a few fixes in the run scripts (issue pending).
Note that running clean-all.sh will currently fail, until the leftover FSI/ directory is removed (see #168). Just remove it locally to test it.

Review

@uekerman Please comment on the mechanism.
@fsimonis Please have a quick look at the scripts: do you see anything problematic?
@davidscn Please try to run 1-2 cases and check that it is working as expected.

@MakisH MakisH self-assigned this Mar 24, 2021
Copy link
Copy Markdown
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

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

* `tools/cleaning-tools.sh`: Contains one function for each solver (e.g. `clean_openfoam`), one shared function to clean the preCICE log files, and one function `clean_tutorial`.

* `<tutorial>/<case>/clean.sh`: Only triggers the right `clean_<solver>`.

* `<tutorial>/clean-tutorial.sh`: The same for every tutorial, only triggers `clean_tutorial`.

* `clean-all.sh`: Triggers all `clean-tutorial.sh`, cleaning everything:

* The scripts cannot be started from arbitrary directories (I may implement this, but I don't see much added value). The check we had before (`cd ${0%/*} || exit 1`) is not correct.

Very nice. The only thing I don't like that much is the duplication on the lowest level. Since we have a naming convention, it might be possible to extract the current participant name from the working path. Using symlinks as you already proposed would be a great idea and then you could just have a single script for all cases on the lowest level.

* In most (all?) OpenFOAM cases we have a first step where we copy the backup `0.orig/` to `0/`. This is normally not needed, so I left this deletion step out of the function and added it explicitly to each case.

I already had cases, where OpenFOAM complains about missing boundary conditions. So, might depend on the version.

* A few more files are intentionally cleaned up now:
  
  * The `.foam` files are now deleted and not recreated. This should be done by the `run.sh` (there was some motivation for this in the past, but it is not really relevant anymore).
  * The `elastic-tube-3d` previously skipped deleting the OpenFOAM mesh, probably to save time. This was always confusing. I now delete them as well and I will add them to the `.gitignore` later.

No, the reason for the elastic-tube-3d was not to to save time, but to keep the mesh. We have no blockMesh script for the mesh (I guess it has been created with an external software). If you delete it here, it will be gone (or you need to restore it from the git tree).

Still open

* Check if the CalculiX, deal.II, Nutils, SU2, and code_aster are cleaned up completely (I just distilled the previous lists for now).

This should be part of the control step in #160.

Copy link
Copy Markdown
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

I like the mechanism, great stuff 🚀
(I did not test it right now)

Comment thread tools/cleaning-tools.sh Outdated
Comment thread elastic-tube-3d/clean-tutorial.sh Outdated
Comment thread clean-all.sh Outdated
@MakisH
Copy link
Copy Markdown
Member Author

MakisH commented Mar 24, 2021

@davidscn Thank you very much for the detailed comments!

Very nice. The only thing I don't like that much is the duplication on the lowest level. Since we have a naming convention, it might be possible to extract the current participant name from the working path. Using symlinks as you already proposed would be a great idea and then you could just have a single script for all cases on the lowest level.

I would like to keep the deepest level case-specific: it allows to remove special files or do additional tricks (e.g. backup something, then restore it). But I used symlinks for the intermediate level, where all files are identical.

I already had cases, where OpenFOAM complains about missing boundary conditions. So, might depend on the version.

I am not sure what you mean here, but I interpret it as "let's keep it as it is".

No, the reason for the elastic-tube-3d was not to to save time, but to keep the mesh. We have no blockMesh script for the mesh (I guess it has been created with an external software). If you delete it here, it will be gone (or you need to restore it from the git tree).

Good catch, thank you. I renamed polyMesh to polyMesh.orig and treated it similarly to 0.orig in run.sh. This makes the clean.sh the same as any other OpenFOAM clean.sh.

I think we have the same issue in the heat-exchanger, but there we have the Download_meshes script as a backup. I don't expect people to run this multiple times and I would prefer to avoid keeping a large backup.

This should be part of the control step in #160.

I checked and they are working fine: OpenFOAM, CalculiX, deal.II, SU2

Pending After merging:

  • Check FEniCS cleaning
  • Check Nutils cleaning
  • Check code_aster cleaning
  • Adapt also the Quickstart case
  • Update the system tests

@MakisH
Copy link
Copy Markdown
Member Author

MakisH commented Mar 24, 2021

I think the only reliable way to make the scripts work when invoked from any directory would require switching to bash from sh. I am not sure if it is worth the pain sticking to sh, so I would like to postpone this feature to after also considering the run.sh scripts.

. "${BASH_SOURCE%/*}/../../tools/cleaning-tools.sh"

This will not not work on the standard sh or the (default in Ubuntu) dash. I understand that it will also not work on zsh.

OpenFOAM works around this issue by specifying the tools path in an environment variable, but we want to avoid going down that road, as it would make more things unnecessarily complicated for the user.

At the end of the day, I am not sure if the solvers themselves are so portable and nobody ever complained for the tutorials using bash (though this fact is surprising).

Copy link
Copy Markdown
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

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

Very, nice 💥
As I said, I will check the cleaning scripts when checking the tutorials again.

@fsimonis fsimonis self-requested a review March 25, 2021 15:42
@MakisH MakisH merged commit 7d6c6b1 into develop Mar 25, 2021
@MakisH MakisH deleted the cleaning branch March 25, 2021 16:23
uekerman added a commit that referenced this pull request Apr 6, 2021
* Strip case of everything besides simple heat conduction setup.

* Adds precice-config.xml supporting parallel runs.

* Add documentation and nutils case.

* Add complex case. Based on ae77ae5.

* Cleanup and Update to newest version of python bindings.

* Use scalar-valued flux.

* Apply new structure to complex case.

* Remove subcycling.

* Simplify heat nutils to one mesh, make config consistent

* Fix names.

* BCs working.

* Minor restructuring

* Fix initial condition and output.

* Mark nutils case as under construction. See #152.

* Add nutils prototype from 679fe8d.

* Add instructions on running nutils case to README

* Cleanup

* Fix geometry.

* Add convenience scripts and improve output.

* Make output more consistent.

* Update convenience scripts.

* Fix sign error.

* Add monolithic solver for comparison.

* Scale flux by element size.

* Use identical mesh for FEniCS and Nutils.

* Add argument for setting error tolerance.

* Update README.md

* Remove monolithic case.

* Smooth readme of partitioned heat case

* More information on error.

* Provide cleaning scripts according to #169.

* Streamline run and clean scripts.

* Cleanup.

* Align Nutils vtk output format with Paraview

* Make explanation of fenics-nutils error more precise

* Comment, format, and rewrite nutils heat code

Co-authored-by: uekerman <benjamin.uekermann@gmail.com>
Co-authored-by: Benjamin Uekermann <benjamin.uekermann@ipvs.uni-stuttgart.de>
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