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

Update domain.py #63

Merged
merged 5 commits into from
Jun 13, 2023
Merged

Update domain.py #63

merged 5 commits into from
Jun 13, 2023

Conversation

keiyamamo
Copy link
Collaborator

Two main reasons for this PR

  1. There was a problem restarting the problem. The problem should be fixed with this PR. (I tested restarting and it works now)
  2. domain.py was quite complicated to understand without enough documentation. Therefore, I added args/returns as well as underlying assumption for some of the variables that was not explicitly stated.

Please review my comments and feel free to add comments on them!

Kei

@keiyamamo keiyamamo requested a review from dbruneau-mie June 12, 2023 11:10
@keiyamamo keiyamamo marked this pull request as draft June 12, 2023 11:39
@keiyamamo keiyamamo marked this pull request as ready for review June 12, 2023 14:44
@keiyamamo keiyamamo requested a review from jorgensd June 12, 2023 14:51
Copy link
Collaborator

@dbruneau-mie dbruneau-mie left a comment

Choose a reason for hiding this comment

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

Hey Kei, these comments look clear to me

@keiyamamo keiyamamo merged commit 6832ddc into KVSlab:master Jun 13, 2023
@keiyamamo keiyamamo deleted the update_domain branch June 13, 2023 18:31
Copy link
Collaborator

@dbruneau-mie dbruneau-mie left a comment

Choose a reason for hiding this comment

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

Hey Kei, have you checked that these edits work with the following:

  1. Single Solid Region, like the Turek Flag
  2. Multiple Solid regions
  3. Single Fluid region
  4. Multiple Fluid regions
  5. St. Venant Kirchoff material
  6. Mooney Rivlin material
  7. With Robin BC?
    I would like to make sure this doesn't break any of the existing functionality before incorporating this PR.

@keiyamamo
Copy link
Collaborator Author

Hi Daivd,

Good point!
I did test 1, 3, 5, 7, but not 2, 4, 6. Do you have any problems with multiple solid/fluid regions that I can test?

Best,
Kei

@dbruneau-mie
Copy link
Collaborator

Hey Kei, here is a simulation with mutliple fluid regions in the Aneurysm Workflow FSI:
https://github.com/dbruneau-mie/Aneurysm_Workflow_FSI/blob/master/simulations/stenosis_FC/pulsatile_vessel.py

@dbruneau-mie
Copy link
Collaborator

And in the solver itself there is a cylinder with 2 solid regions
https://github.com/KVSlab/turtleFSI/blob/master/turtleFSI/problems/Test_Cylinder/problem_aneu_2solid.py

@keiyamamo
Copy link
Collaborator Author

keiyamamo commented Jun 13, 2023

Hi @dbruneau-mie

Thanks for pointing out! I just reverted the change and will re-open the pull-request after I test the changes.
We should consider adding multiple fluid/solid regions as a part of test...

Best,

@dbruneau-mie
Copy link
Collaborator

Yes, I think we need more tests for the solver. A restart test would also be very useful

@keiyamamo
Copy link
Collaborator Author

I did work on that, but found that the problem is multi-folded, meaning we need to fix time loop, drag/lift evaluation and not-designed-to restart. But I will work on this now that it turned out that it is important we have well designed test to avoid manually performing many kinds of test.

@keiyamamo keiyamamo mentioned this pull request Jun 15, 2023
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