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

Restructure quickstart #145

Merged
merged 23 commits into from
Apr 12, 2021
Merged

Conversation

davidscn
Copy link
Member

@davidscn davidscn commented Jan 8, 2021

Open TODOs:

  • update README
  • update image (essentially an older TODO)

@davidscn davidscn self-assigned this Jan 8, 2021
@davidscn davidscn linked an issue Jan 8, 2021 that may be closed by this pull request
@davidscn davidscn marked this pull request as draft January 19, 2021 11:29
@davidscn
Copy link
Member Author

Ready for review, but still in draft in order to avoid an accidental merge.

@davidscn davidscn force-pushed the restructure-quickstart branch from ab358ca to a88fce0 Compare February 5, 2021 08:27
@davidscn davidscn mentioned this pull request Feb 5, 2021
@MakisH MakisH self-requested a review March 3, 2021 10:23
@uekerman
Copy link
Member

uekerman commented Mar 3, 2021

Please remember to not merge this before everything else is merged. Otherwise we change the content here: https://www.precice.org/quickstart.html
The webpage points to the restructure branch.

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.

A few minor comments, mostly very picky changes in the formulations and a rather important bug prevention in the scripts.

I ran the case and I observed that:

  • ✔️ The case is running
  • ❌ The removeObsoleteFolders.sh needs to remove the cd Fluid and the cd ...
  • ✔️ The plotDisplacement.sh works
  • ✔️ ParaView works.
  • ❔ I still see a kind-of "explosion" in the second half of the simulation (higher-frequency oscillation). Maybe we could add a note that this is expected and why.
  • clean.sh needs a few fixes (see review)

Looks very nice already! 🚀

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.

A few more minor changes in the README.md (sorry, I only saw them now)

@davidscn
Copy link
Member Author

I added now a result figure. Feel free to comment on it. Other than that, the PR should be ready.

@davidscn davidscn marked this pull request as ready for review March 11, 2021 07:57
@MakisH
Copy link
Member

MakisH commented Mar 11, 2021

Please remember to not merge this before everything else is merged. Otherwise we change the content here: https://www.precice.org/quickstart.html
The webpage points to the restructure branch.

Just a reminder

@davidscn davidscn mentioned this pull request Mar 11, 2021
@uekerman uekerman changed the base branch from restructure to develop March 17, 2021 11:49
This was referenced Mar 17, 2021
@davidscn
Copy link
Member Author

@MakisH merging develop was a bit of a challenge. but you can cross-review the cleaning/running now as discussed.

@MakisH
Copy link
Member

MakisH commented Apr 12, 2021

I looked at the commit with the cleaning/running and the latest state and it looks good 👍.

@davidscn davidscn merged commit 1320b7b into precice:develop Apr 12, 2021
@davidscn davidscn deleted the restructure-quickstart branch April 12, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge and port quickstart to new tutorials structure
3 participants