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

Add solid fake for perpendicular flap tutorial #541

Merged
merged 8 commits into from
Aug 19, 2024

Conversation

NiklasVin
Copy link
Collaborator

I added a solid fake implemented in Python to the perpendicular flap tutorial, similar to the already existing fluid-fake.

@MakisH
Copy link
Member

MakisH commented Jun 24, 2024

Is this ready for review?
Don't forget to add it into the README of the tutorial. I can generate some of the plots when reviewing.

I will fix the markdownlint, it is failing also elsewhere.

@MakisH
Copy link
Member

MakisH commented Jun 27, 2024

@NiklasVin please rebase/merge from develop, the markdownlint issue is now fixed.

@NiklasVin
Copy link
Collaborator Author

Is this ready for review? Don't forget to add it into the README of the tutorial. I can generate some of the plots when reviewing.

I will fix the markdownlint, it is failing also elsewhere.

Yes, now this should be ready for review.

@MakisH
Copy link
Member

MakisH commented Jul 2, 2024

@NiklasVin can you please enable edits by maintainers? It's a checkbox on the right.

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 checked the case with fluid-openfoam. Generally runs and VTK output looks good. Only a minor point is left + some readability comments.

perpendicular-flap/solid-fake/fake.py Outdated Show resolved Hide resolved
perpendicular-flap/solid-fake/fake.py Show resolved Hide resolved
Comment on lines 10 to 13
# first, get displacement independent of x, only dependent on y and t
# maximal displacement at flap tip should be 0.5
# initially, flap's displacement is 0
x_displ = np.minimum(((np.sin(3 * np.pi * t + np.arcsin(-0.95)) + 0.95) / 8) * y / flap_tip_y, 0.5 * y / flap_tip_y)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# first, get displacement independent of x, only dependent on y and t
# maximal displacement at flap tip should be 0.5
# initially, flap's displacement is 0
x_displ = np.minimum(((np.sin(3 * np.pi * t + np.arcsin(-0.95)) + 0.95) / 8) * y / flap_tip_y, 0.5 * y / flap_tip_y)
# first, get displacement independent of x, only dependent on y and t
# initially, flap's displacement is 0
max_x_displacement = 0.5
x_displ = np.minimum(((np.sin(3 * np.pi * t + np.arcsin(-0.95)) + 0.95) / 8) * y / flap_tip_y, max_x_displacement * y / flap_tip_y)

I would generally try to avoid raw numbers and rather use descriptive names. This often allows you to completely save some comments.

What does the 0.95, the 3 * np.pi and the /8 do? Generally, I think if the formula you are using here works we can keep it. But a brief comment about the general rationale behind the arcsin and sin would be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added some comments that hopefully clarify the idea behind the formula

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.

Thank for the changes! Looks good now. Let's get this merged 🎉

@BenjaminRodenberg BenjaminRodenberg merged commit bb17d2c into precice:develop Aug 19, 2024
1 check passed
@NiklasVin NiklasVin deleted the solid-fake branch August 19, 2024 12:42
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.

3 participants