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

Added NS incompressible and Euler equation. Also fixed tessellation, so now will work on windows via docker or pip #30

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

chaous
Copy link

@chaous chaous commented Jun 27, 2023

In some cases it is better to use incompressible NS and Euler equation uses way less memory

@NickGeneva NickGeneva added the enhancement New feature or request label Jun 27, 2023
(only docker tested, however supposed to work with pip too (will try to test in the future)) and cannot calculate sdf derivative in windows currently
added notes for windows and pip fixes
@chaous chaous changed the title Added NS incompressible and Euler equation Added NS incompressible and Euler equation (also fixed tessellation, so now will work on windows via docker or pip) Jul 11, 2023
@chaous
Copy link
Author

chaous commented Jul 11, 2023

I fixed tessellation, so now will work on windows via docker or pip. I used mesh_to_sdf instead of pysdf

@chaous chaous changed the title Added NS incompressible and Euler equation (also fixed tessellation, so now will work on windows via docker or pip) Added NS incompressible and Euler equation. Also fixed tessellation, so now will work on windows via docker or pip Jul 16, 2023
@michaltakac
Copy link
Contributor

Looking forward for reviews, since our team cannot upgrade to modulus:23.05 due to missing pysdf library

README.md Outdated
pip install trimesh
pip install mesh_to_sdf
```
Tesselation will work because insted of using pysdf I used mesh_to_sdf (I have not yet implemented sdf derivatives yet, so some function will not work on windows (It's only a few functions for visualisation difference with real data (as far as I know)))
Copy link

Choose a reason for hiding this comment

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

Minor typo, should be instead

@@ -339,6 +339,90 @@ def __init__(self, T, dim=3, time=True):
normal_x * T.diff(x) + normal_y * T.diff(y) + normal_z * T.diff(z)
)

class NavierStokesIncompressible(PDE):
'''Cystom implementation of incompressible NS eqyastyans'''
Copy link

Choose a reason for hiding this comment

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

Should be custom and equations



class Euler(PDE):
'''Cystom implementation of compressible Euler eqyastyans'''
Copy link

Choose a reason for hiding this comment

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

Typo as above


# density
if isinstance(rho, str):
rho = Function(rho)(*input_variables)
Copy link

Choose a reason for hiding this comment

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

If this incompressible, then surely rho must be a constant? Therefore raising an exception would make more sense

Copy link
Author

Choose a reason for hiding this comment

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

You probably correct, I will fix it

fixed typos in README.md
fixed typos and added Exeption for incompresiible rho
@chaous
Copy link
Author

chaous commented Oct 3, 2023

Implemented all suggestions

@chaous chaous requested a review from cfd1 October 3, 2023 12:56

# density
if isinstance(rho, str):
raise Exception("rho myst be number")
Copy link

Choose a reason for hiding this comment

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

typo must

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@cfd1
Copy link

cfd1 commented Oct 4, 2023

I need to check the code when I get a chance, but how is this different from calling the Navier Stokes with a constant rho?

@chaous
Copy link
Author

chaous commented Oct 5, 2023

In classical CFD, for better results, incompressible form is used for low Mach numbers. I was able to achieve better results with an incompressible equation for PINNs than with a compressible equation but with constant rho

@chaous chaous requested a review from cfd1 October 5, 2023 10:48
@cfd1
Copy link

cfd1 commented Oct 5, 2023

Just for the record I can’t review or merge your work.

@cfd1
Copy link

cfd1 commented Oct 5, 2023

If you can it would be good to have an example of your changes working. You could use the lid driven cavity for the incompressible formulation. I’m not sure if there is a compressible example with shockwaves to show off the Euler equations.

@YouqiongLiu
Copy link

If you can it would be good to have an example of your changes working. You could use the lid driven cavity for the incompressible formulation. I’m not sure if there is a compressible example with shockwaves to show off the Euler equations.

Can you work via pip?

@YouqiongLiu
Copy link

YouqiongLiu commented Mar 20, 2024

I fixed tessellation, so now will work on windows via docker or pip. I used mesh_to_sdf instead of pysdf

e.g modulus-sym\examples\geometry\tessellated_example.py
your fixed tessellation can work, but the following warning appears.
/modulus/sym/geometry/tessellation.py:179: RuntimeWarning: divide by zero encountered in divide np.full(x.shape, triangle_areas[index] / x.shape[0])

@chaous
Copy link
Author

chaous commented Mar 23, 2024

I fixed tessellation, so now will work on windows via docker or pip. I used mesh_to_sdf instead of pysdf

e.g modulus-sym\examples\geometry\tessellated_example.py your fixed tessellation can work, but the following warning appears. /modulus/sym/geometry/tessellation.py:179: RuntimeWarning: divide by zero encountered in divide np.full(x.shape, triangle_areas[index] / x.shape[0])

It appears on on regular tessellation too. This warning can be ignored

@ktangsali
Copy link
Collaborator

Hi @chaous , sorry for the delay in getting attention to this PR. Thanks for the contribution! Can we split this PR into two? One that adds equations and the other that adds changes to tessellation? Reviewing the tessellation might take some time, but we can get the equations added much sooner. Also, if you can add an example to go along with your equation additions that can demonstrate potential speed-up/improvement in accuracy, that would be helpful.

@chaous
Copy link
Author

chaous commented Mar 28, 2024

Hi @chaous , sorry for the delay in getting attention to this PR. Thanks for the contribution! Can we split this PR into two? One that adds equations and the other that adds changes to tessellation? Reviewing the tessellation might take some time, but we can get the equations added much sooner. Also, if you can add an example to go along with your equation additions that can demonstrate potential speed-up/improvement in accuracy, that would be helpful.

I split my pull request into two and added examples
Equations #142
tessellation #143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants