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

[bug] Fix stable fluid BFECC advection #1764

Merged
merged 8 commits into from Sep 26, 2020
Merged

[bug] Fix stable fluid BFECC advection #1764

merged 8 commits into from Sep 26, 2020

Conversation

Eydcao
Copy link
Contributor

@Eydcao Eydcao commented Aug 24, 2020

Related issue = #1757

In this PR I plan to fix the BFECC advection in stable_fluid.py. Seems that there are several problems tangled inside this one. So let's try to solve them one-by-one. I may need your @yuanming-hu and @archibate help to review if the errors are from the BFECC or the PPE solving. Thanks a lot,

  • Fixed the correction term's symbol.
  • Reset Poisson solver to jacobi_single.
  • Fixed the whole advect_bfecc function.
  • Fixed the int() (with math.floor()) which irritates when input is negative

[Click here for the format server]


@codecov
Copy link

codecov bot commented Aug 24, 2020

Codecov Report

Merging #1764 into master will decrease coverage by 0.84%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1764      +/-   ##
==========================================
- Coverage   43.42%   42.58%   -0.85%     
==========================================
  Files          44       44              
  Lines        6010     6185     +175     
  Branches     1041     1072      +31     
==========================================
+ Hits         2610     2634      +24     
- Misses       3249     3397     +148     
- Partials      151      154       +3     
Impacted Files Coverage Δ
python/taichi/lang/__init__.py 42.44% <0.00%> (-11.16%) ⬇️
python/taichi/lang/impl.py 64.50% <0.00%> (-0.73%) ⬇️
python/taichi/main.py 23.03% <0.00%> (-0.54%) ⬇️
python/taichi/lang/matrix.py 64.73% <0.00%> (-0.30%) ⬇️
python/taichi/lang/ops.py 43.89% <0.00%> (-0.19%) ⬇️
python/taichi/testing.py 78.12% <0.00%> (ø)
python/taichi/misc/gui.py 0.00% <0.00%> (ø)
python/taichi/misc/image.py 41.26% <0.00%> (+0.09%) ⬆️
python/taichi/lang/common_ops.py 69.26% <0.00%> (+0.21%) ⬆️
python/taichi/lang/expr.py 61.95% <0.00%> (+0.28%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6302ec...ae0c782. Read the comment docs.

examples/stable_fluid.py Outdated Show resolved Hide resolved
examples/stable_fluid.py Outdated Show resolved Hide resolved
intermedia_qf[i, j] = bilerp(qf, p)

ti.cache_read_only(intermedia_qf, qf, vf)
for i, j in vf:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the key problem about separating bfecc to two offload?

Copy link
Contributor Author

@Eydcao Eydcao Aug 26, 2020

Choose a reason for hiding this comment

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

Yes. To be more precise, we need a forward advection (backtrack + interpolate) to get an inter-media quantity field first, then a backward advection to (backtrace in -dt direction + interpolate) to get the field at the original time stamp (assuming there is no error). Finally, we got the error and fix the inter-media field.

Since for the second one (backward advection) we need the whole inter-media field ready, it's necessary to run it in a separate another loop and store it already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But to be noticed, jacobbi_dual caused serious artifacts with the new bfecc.

I will post detailed comparisons between combination of different methods when I get a minute.

@yuanming-hu yuanming-hu changed the title [bug] stable fluid's bfecc advection [bug] Stable fluid's bfecc advection Aug 26, 2020
examples/stable_fluid.py Outdated Show resolved Hide resolved
examples/stable_fluid.py Outdated Show resolved Hide resolved
examples/stable_fluid.py Outdated Show resolved Hide resolved
Co-authored-by: 彭于斌 <1931127624@qq.com>
@Eydcao
Copy link
Contributor Author

Eydcao commented Aug 26, 2020

Comparison List:

  1. Old bfecc

old

  1. New bfecc with jacobi_dual

video

  1. New bfecc with jacobi_single

video

@Eydcao
Copy link
Contributor Author

Eydcao commented Aug 26, 2020

  1. New bfecc with mgpcg

video

This is super interesting~@archibate

@archibate
Copy link
Collaborator

Thanks for all the hard work here! I think I found the reason now.

@Eydcao
Copy link
Contributor Author

Eydcao commented Aug 26, 2020

After fixing the irrigating int() issue in interpolation, the boundary artifact is gone. And the new bfecc works nicely with jacobi_single. As the jacobi_double or mgpcg concerns other issues in PPE solving, I will leave them there to solve :)

Will organize and make it ready for review

@Eydcao Eydcao marked this pull request as ready for review August 26, 2020 16:44
examples/stable_fluid.py Outdated Show resolved Hide resolved
examples/stable_fluid.py Outdated Show resolved Hide resolved
@@ -326,6 +338,9 @@ def reset():
dyes_pair.cur.fill(0)


v_mngr = ti.VideoManager('./', framerate=24, automatic_build=False)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

@@ -326,6 +338,9 @@ def reset():
dyes_pair.cur.fill(0)


v_mngr = ti.VideoManager('./', framerate=24, automatic_build=False)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

@archibate
Copy link
Collaborator

Please do not use apply suggestions for large modification, always keep in mind that all our actions on GitHub will be sent into
322 people's mailbox. See https://github.com/taichi-dev/taichi/watchers.

@Eydcao Eydcao requested a review from archibate August 28, 2020 05:24
@yuanming-hu yuanming-hu changed the title [bug] Stable fluid's bfecc advection [bug] Fix stable fluid's BFECC advection Sep 26, 2020
@yuanming-hu yuanming-hu changed the title [bug] Fix stable fluid's BFECC advection [bug] Fix stable fluid BFECC advection Sep 26, 2020
@yuanming-hu yuanming-hu merged commit 206304e into taichi-dev:master Sep 26, 2020
@yuanming-hu yuanming-hu mentioned this pull request Sep 26, 2020
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.

None yet

4 participants